tP comments dashboard

Sorted based on the number of comments given to others' PRs, but also showing comments on own PRs and other comments given.

[This page was last updated on Oct 12 2020]

Do change the USERGUIDE_URL in HelpWindow.java as well!

Switch to use logger

Switch to use logger, same for other 2 instances below

Switch to use logger

Remove

"Clears the item list"?

Would it be better to default quantity to 0 and description/location to none if user doesn't key them in? Currently they act as compulsory fields.

Javadocs

Extra lines

change address-book

Javadocs

change comment title

person -> item

Should these be locationIds and recipeIds?

Should these method names be getLocationIds and getRecipeIds?

use this format instead, replace with the fields we have

plural forms

extra lines

address book -> item list?

Javadocs

Make this comment appropriate pls 😃

Javadocs

the right side has less equal signs 😦

change address book to item list or something similar

same for this segment

change logger message

itemToAdd might be better

extra lines

item -> item and location

person -> item

extra lines

ItemList->RecipeList

item list file path

of recipes

recipe list file path

will fail checkstyle

item -> recipe

checkstyle

can consider shorter prefix

for clarity can change to "Recipe ID is out of range"

should this be {@code Item}?

catch specific exception

add comment above this line specifying that this is for the delr command

conform -> conform to

extra lines

this constructor might be misleading, would it be better to modify the existing one?

for instance

'''

if (!isDeleted) {

idCounter++;

}

'''

in the existing constructor might make it usable as an equivalent to this one

remove or change to logger statement

Name typo (?), img not updated

[stephentan]

change names in [ ]s to be consistent styling

img not updated

address book -> item/recipe

these steps as well

persons -> items/recipes

actual pictures pls 😃

should this be 3a and 4a?

recipe

standardise with README, use InvInator.jar or change README to inventoryinator.jar

add line back

via the issue tracker of this repository, with link to it

keep names consistent - full name or given name

rephrase to "what is going on under the hood"

should this be "deli stone" for the moment?

delr -n -i ?

should locations be included as well?

del -> deli

same for this line

delete -> deli/delr

mark as TODO for easier search

optionally provide a short intro on how we structured our personas (explain that our app will help transition the before scenario to the after scenario, etc.)

include option to use

'''

java -jar InvInator.java

'''

contacts -> items

mark as TODO

should we change to deli and delr for now?

should we split del -> deli + delr?

should we split del -> deli + delr here?

i think we should specify that they need to overwrite multiple files, since our storage for each class is in separate files

change titles for each of these files

Should this use the NOT_FOUND message instead? User does not need to know our implementation

'''

itemList.removeIf(x -> !x.getName().equals(productName) || x.isDeleted());

'''

in line 43 looks like it already ensures the item won't be deleted

Suggest to change phrasing to something like

"soft deletes recipes that contain deleted item as product or ingredient"

DeleteRecipeCommand -> DeleteItemCommand

use proper punctuation please

and all recipes associated with the item

please space the methods in this file

spacing

remove this line

@code item

spacing

proper punctuation please

spacing

use block comment

fix spacing in this file

spacing

space out these tests

re-add spaces in this file

fix spacing

spacing

more clear explanation needed

spacing

should these be containsMultipleWordsIgnoreCase?

remove line

is it possible to test results of command? eg. if no such item exists, or check that the item being shown is indeed the right item

import from the file with existing modelstub

deceptive naming

try to remove if not too necessary, is an unusual class to have

use isEmpty

Might be better to not have "will you like to add some?" if there's no direct button to do so

use isEmpty

should this file be added?

Just "No items match the search string." should suffice here

Done in https://github.com/AY2021S1-CS2103T-F13-1/tp/pull/17

Done in https://github.com/AY2021S1-CS2103T-F13-1/tp/pull/16

Issue #30 opened as per review.

resolved by #31

Pushed to future PRs:

LocationList current defaults to Optional.empty, to be fixed and tested.
RecipeParserUtilTest incomplete, does not test for ingredient-related parsing.

I think we can remove this test case.

'''suggestion

    // Keywords match phone and email, but does not match name

'''

Should remove INVALID_ADDRESS and VALID_ADDRESS from constant declarations above as well.

Is there a particular reason as to why this test case was removed? Perhaps we can use a different invalid value instead of removing the entire test case.

Should probably remove the above

'''

@FXML

private Label address;

'''

as well.

'''suggestion

platform for tutors to handle all of their administrative matters.

'''

'''suggestion

Tutor's Pet displays your upcoming lessons so that you will never forget about them again.

'''

Not sure if this would sound more in line with the other points since the word 'your' was used in both the other.

'''suggestion

Tutor's Pet allows you to mark your students' attendance and participation for each lesson.

'''

Nice catch!

'''suggestion

public static final String MESSAGE_SUCCESS = "Listed all students.";

'''

Perhaps we can update the Tag examples to something that's more student-tutor relationship like. Maybe something like how the student is performing in class.

This line shouldn't change.

Similar here, would be good to update the example tags.

Would be good to update this test constant as well.

Command word should be in kebab case.

Command word should be in kebab case. Perhaps can change the title to "Listing all students within a class".

We should retain the generated table of contents instead of using simple bullet points.

I think the second add is redundant.

Ellipsis should remain outside of the square brackets since users need to repeat the prefix as well.

I don't think we are supporting the separation of CLASS_NAME from MODULE_CODE?

Need to remove the address field in this section as well and update the tag prefix.

Need to update tag prefix here as well.

I don't think name, telegram handle and email are optional fields.

We need to rethink the command syntax for this operation. The parsing of arguments will be troublesome in this format.

For now maybe we can leave this feature out of the UG.

Kind of weird I'm a Project Advisor in the project I'm working on. Maybe Lead Developer would make more sense, but I'll change that in my own AboutUs update later.

'''suggestion

'add''n/John Doe t/@johndoe e/johnd@example.com tag/student' : Adds a student named 'John Doe' to the application.

'''

The second example should include more than one 'tag' to show the possible multiplicities.

'''suggestion

e.g. '[tag/TAG]…' can be used as ' ' (i.e. 0 times), 'tag/student', 'tag/Average tag/TA Candidate' etc.

'''

'''suggestion

Format: 'add n/NAME t/TELEGRAM_HANDLE e/EMAIL [tag/TAG]…'

'''

'''suggestion

Format: 'edit INDEX [n/NAME] [t/TELEGRAM_HANDLE] [e/EMAIL] [tag/TAG]…'

'''

Not sure if this is the best implementation since the '/by' prefix will have no values associated with it when parsed by the 'ArgumentTokenizer'

'''suggestion

'list-students c/CS2103T Tutorial T10'

'''

'''suggestion

Add Student | 'add n/NAME t/TELEGRAM_HANDLE e/EMAIL [tag/TAG]…'
e.g., 'add n/John Doe t/@johndoe e/johnd@example.com tag/student'

'''

'''suggestion

Edit Student | 'edit INDEX [n/NAME] [t/TELEGRAM_HANDLE] [e/EMAIL] [tag/TAG]…'
e.g.,'edit 2 n/James Lee e/jameslee@example.com'

'''

'''suggestion

List Students in a Class | 'list-students c/CLASS_NAME'
e.g., 'list-students c/CS2103T Tutorial T10'

'''

'''suggestion

'''

'''suggestion

Links an existing student to an existing class in the application.

'''

'''suggestion

'list' followed by 'link s/1 c/2' links the 1st student in the application to the 2nd class in the application.
'find Betsy' followed by 'link s/1 c/2' links the 1st student in the results of the 'find' command to the 2nd class in the application.

'''

'''suggestion

Linking a student to a class: 'link'

'''

Should we update the rest of the use cases to use full stops as well?

Hmm, I think summary should remain after the detailed explanation, since it serves so summarise the above mentioned information.

In and A shouldn't be capitalised in the title.

I think we should retain the present participle form of the verb? The CS2101 set of slides for "Writing User Guide" also seems to favour retaining the present participle form of verbs, although it does not seem explicitly mentioned.

'''suggestion

3a. No students found.

'''

Would be good to mention that it is a message, otherwise might be confusing how to show an absence of something.

Should we keep it consistent and use delete instead of clear?

Technically Tutor's Pet can return more than one student found, so maybe it would be good to mention that it shows all matching students.

Applies to finding classes as well.

'''suggestion

Command overview

'''

'''suggestion

Clearing all students : 'clear-student'

'''

'''suggestion

Clearing all classes : 'clear-class'

'''

'''suggestion

| UC13 | Link a student to a class |

| UC14 | Unlink a student from a class |

'''

'''suggestion

Use case: UC14 - Unlink a student from a class

'''

'''suggestion

Use case: UC13 - Link a student to a class

'''

'''suggestion

| UC12 | Clear all classes |

| UC13 | Link a student to a class. |

| UC14 | Unlink a student from a class |

'''

'''suggestion

    assertParseFailure(parser, NAME_DESC_BOB + VALID_PHONE_BOB + EMAIL_DESC_BOB,

'''

'''suggestion

    assertParseFailure(parser, VALID_NAME_BOB + PHONE_DESC_BOB + EMAIL_DESC_BOB,

'''

'''suggestion

    assertParseFailure(parser, NAME_DESC_BOB + PHONE_DESC_BOB + VALID_EMAIL_BOB,

'''

'''suggestion

public static final String MESSAGE_SUCCESS = "All students in Tutor's Pet have been cleared!";

'''

Perhaps we can rename this to getAddStudentCommand(Student student)

'''suggestion

public void parseCommand_editStudent() throws Exception {

'''

'''suggestion

public void parseCommand_deleteStudent() throws Exception {

'''

'''suggestion

public void parseCommand_addStudent() throws Exception {

'''

'''suggestion

public void parseCommand_clearStudent() throws Exception {

'''

'''suggestion

public void parseCommand_findStudent() throws Exception {

'''

'''suggestion

public void parse_validArgs_returnsFindStudentCommand() {

'''

'''suggestion

public void parse_validArgs_returnsDeleteStudentCommand() {

'''

I don't think we should allow the explicit setting or getting of UUID in EditStudentDescriptor since it would then no longer be editing the same student.

The editedUuid should always be that of the studentToEdit.

I think it would be good to set a default UUID for each student in the tests and not rely on randomly generated data. This way it would be more predictable and testable.

I don't think there is a need for EditStudentDescriptor to have a uuid field.

'''suggestion

    requireAllNonNull(uuid, name, phone, email, tags);

'''

It would be good to edit the constructors in the TypicalStudents file as well to ensure the test constants are truly identical.

'''suggestion

'''

'''suggestion

private static ModuleClass createEditedModuleClass(ModuleClass moduleClassToEdit,

        EditModuleClassDescriptor editModuleClassDescriptor) {

'''

'''suggestion

Parses input arguments and create a new EditModuleClassCommand object.

'''

'''suggestion

 * Parses {@code userInput} into a {@code StudentNameContainsKeywordsPredicate}.

'''

There's quite a bit of overlap here, would be good to deduplicate into:

'''suggestion

public static final Index INDEX_FIRST_ITEM= Index.fromOneBased(1);

public static final Index INDEX_SECOND_ITEM = Index.fromOneBased(2);

public static final Index INDEX_THIRD_ITEM = Index.fromOneBased(3);

'''

'''suggestion

A utility class for ModuleClass.

'''

Seeing this template always makes me feel like they have already foreshadowed that we should do undo/redo features.

'''suggestion

Contains integration tests (interaction with the Model) and unit tests for

'''

'''suggestion

A UI component that displays information of a {@code ModuleClass}.

'''

Since we seem to be standardising to capitalise UI.

'''suggestion

// Independent UI parts residing in this UI container

'''

It should be A UI because in both user interface and UI the word/abbreviation is pronounced as though it starts with the consonant y.

'''suggestion

A UI component that displays information of a {@code Student}.

'''

'''suggestion

-fx-background-color: transparent, #274156, transparent, #274156;

'''

I think it would be best to define the return type as Label since we do in fact return a Label.

'''suggestion

 * Converts this Jackson-friendly adapted class object into the model's {@code ModuleClass} object.

'''

'''suggestion

 * Converts a given {@code UUID} into this class for Jackson use.

'''

'''suggestion

 * Converts this Jackson-friendly adapted UUID object into the model's {@code UUID} object.

'''

'''suggestion

 * @throws IllegalValueException if adapted UUID is null.

'''

I think it would be a good idea to introduce some checks for invalid UUIDs since the user can go into the json file to edit entries manually.

I would agree with @junlong4321 on this, it would be good to isolate any possible failure to just students.

While in invalidClassTutorsPet.json, students would be useful in the future when we implement validation for students who are registered in classes.

I agree with @ruixuantan. It would be safer since users can head into the json save file and modify its contents manually.

I think if we are fine with breaking the abstraction barrier, in some sense we already have since we are dealing with UUID objects instead of strings, then it is okay to change all references to studentUuids instead, including in the model.

Naming here might be a bit confusing since uuid is a object type itself.

'''suggestion

private final String uuidString;

'''

'''suggestion

 * Constructs a {@code JsonAdaptedModuleClass} with the given class details.

'''

'''suggestion

 * Converts a given {@code ModuleClass} into a {@code JsonAdaptedModuleClass} for Jackson use.

'''

'''suggestion

 * @throws IllegalValueException if there were any data constraints violated in the adapted class.

'''

'''suggestion

 * Creates a {@code ModuleClassBuilder} with the default {@code Name} and empty {@code studentUuids} list.

'''

'''suggestion

public static final UUID ALICE_UUID = ALICE.getUuid();

public static final UUID BENSON_UUID = BENSON.getUuid();

'''

Can these two methods be made private?

I would advise against adding the class names as Tags since it might confuse our first time users into thinking that linking students to classes would automatically add the classes as Tags.

Perhaps it would be a better idea to declare the students and classes as static members and then have the two methods return a list of the static members.

Also, I think the two getter methods can be made private.

'''suggestion

    // different keyword -> returns false

'''

'''suggestion

 *

 * @throws ParseException if the user input does not conform the expected format.

'''

NIT: It might be a good idea to leave a class or two for manual adding when testing.

nit

'''suggestion

    FindModuleClassCommand expectedFindModuleClassCommand =

            new FindModuleClassCommand(new NameContainsKeywordsPredicate<>(Arrays.asList("CS1101S", "Tutorial")));



    // no leading and trailing whitespaces

    assertParseSuccess(parser, "CS1101S Tutorial", expectedFindModuleClassCommand);

'''

'''suggestion

 * Additionally, removes all {@code Student UUID}s in each {@code ModuleClass}.

'''

'''suggestion

/**

 * Removes all {@code Student}s from the student manager.

 * Also removes all {@code Student UUID}s from each {@code ModuleClass}.

 */

'''

Perhaps it might be a good idea to have the same name for both methods (in Model and in TutorsPet)?

Perhaps '{@code Student UUID}s' would be more accurate since '{@code studentUuids}' refers to the list itself rather than its contents.

'''suggestion

/**

 * Removes all {@code Student UUID}s from every {@code ModuleClass} in the class list.

 */

'''

Is there any particular reason why we choose ClearModuleClassCommand as the different type as opposed to some other primitive type like an integer?

'''suggestion

    model.deleteAllModuleClasses();

'''

'''suggestion

private static final int NUMBER_OF_COLOURS = COLOURS.length;

'''

Should we be using the American spelling of Colour instead?

I think we should retain the name listOfTags or tagList instead? Although the field was declared as static final, it is not exactly a constant in essence since the contents of the list can change.

I think it would be wise to follow the following categorisation of constants as given by the excerpt below from the Google Style Guide.

'''suggestion

    requireNonNull(toCheck);



    return internalList.stream().anyMatch(toCheck::hasSameUuid);

'''

'''suggestion

    requireNonNull(student);



    return students.containsUuid(student);

'''

Instead of reading students once again, perhaps we may want to use the existing information stored within tutorsPet instead?

We can also document this precondition in the JavaDoc to state that the tutorsPet passed in should have its students loaded prior.

Just wondering, is this "feature" extended to the native UUID stored in each JsonAdaptedStudent as well?

'''suggestion

 * If Tutor's Pet encounters duplicate {@code Student UUID}s in a class, it will not load

 * duplicate {@code UUID}s into the model. Hence, Tutor's Pet should still boot up successfully.

'''

'''suggestion

 * Ensures that Tutor's Pet will not be able to boot up given an incomplete {@code UUID}.

'''

'''suggestion

 * Ensures that Tutor's Pet will not be able to boot up although Java tries

 * to pad missing {@code UUID} digits in broken {@code UUID}s with zeros.

'''

'''suggestion

 * Prevents Tutor's Pet from booting up if two or more {@code Student}s are found with the same {@code UUID}.

'''

'''suggestion "Invalid student(s) found in class(es)."; '''

In view of the change, I would recommend changing the test case to remove a student that was not previously part of any class. This way, we would only be changing the student list and would be a better representation of the test case.

TIL, didn't know that.

Quite interesting explanation here also: English StackExchange

The JavaDoc was adapted from an equivalent JavaDoc in UniqueStudentList.

I would be more inclined to retain both in their current expressions.

This JavaDoc was directly adapted from a corresponding JavaDoc in the existing UniqueStudentList class as well. As such, I would be more inclined to retain the current expression since it is consistent with existing code.

The backing list refers to the internalList while unmodifiable means that any attempted modifications on the list would result in an UnsupportedOperationException being thrown.

'''suggestion

    // workaround as storage functionality for ModuleClasses has not been implemented

'''

'''suggestion

    // workaround as storage functionality for ModuleClasses has not been implemented

'''

Agree with @junlong4321 for consistency.

This JavaDoc was directly adapted from a corresponding JavaDoc in the existing UniqueStudentList class, so I would be more inclined to retain the current expression since it is consistent with existing code.

'''suggestion

'''

As these informal list points are not complete sentences and are fragments, I would be more inclined to leave the full stop out?

Oops, that slipped through my manual search and replace.

I think for this case, it would be okay to not include an empty line above the comment since it is the first line in the method body. It will also be more in line with existing code.

Good idea, added in!

Resolved instead by changing to lowercase throughout, more in line with comments found above.

Because of the need to include the CURRENT_INDICATOR for a particular entry, which is rather specific to ViewHistoryCommand, the method in StringUtil would be oddly specialised for a method in the commons package.

Instead, I think I would abstract out the creation of the message to a helper function that resides within the ViewHistoryCommand class.

Can you rebase and then should be good to go.

Not sure if we need to get approval before we merge this in since we are trying to use a new module?

This seems to be missing the saving of UUIDs to local storage, is that planned for a separate PR?

Closed with #61.

Closed with #62.

Resolved in #68.

Closed with #55.

@ruixuantan could I also get your opinion on the test cases written for the new features?

Should we standardize comments? One possible style:

Separate-line comment with whitespace, singular verb, and uppercase 1st character.

'''

// Parses user input...

Command command = addressBookParser.parseCommand(commandText);

'''

Same-line comment with whitespace, singular verb, and lowercase 1st character.

'''

Command command = addressBookParser.parseCommand(commandText); // parses user input...

'''

We could follow any other style too. Just thought it might be better for everyone to stick to 1 for consistency.

Sure, we can restore this when we complete EditCommand.

Really like the regex here. It is simple and effective.

I know this is a bit late, but do you think we should change AddPersonCommand to AddPatientCommand?

I think our Person refers specifically to Patient since we have the periodOfStay field. If we are adding a Staff class in the future, it may be a little misleading since Staff is a Person too.

What do you think?

Since we are using Patient now, maybe we can rename some of these variables?

Perhaps from MESSAGE_INVALID_PERSON_DISPLAYED_INDEX to MESSAGE_INVALID_PATIENT_DISPLAYED_NAME.

Also, do you think we should change our usage of "person" to "patient"? 🤔

i.e. "The patient name..." instead.

I like the use of guard clauses in this method. Makes the code much cleaner 😀

Great work on writing the unit tests! For v1.1, we might not need it but getting it done earlier is a nice bonus.

Maybe it could be invalidPatientName instead? I know it might be a little troublesome to change all references to Person to Patient. I can help with that if you would like.

For separate-line comments, do you think we should keep the first character in uppercase?

The Java coding standard we are using does not explicitly mention it but seems to follow that. I think comments that start with keywords may not need to be capitalized though (e.g. null).

I think "list of patient/all patients that match the specified criteria" may be clearer?

Great use of Optional here!

Do you think an Enum may be preferable here since there are a limited number of possible values?

Using 'int' is not very scalable. If we have 10 different search conditions, we would need 10 'int', and it can be hard to keep track of these variables.

Perhaps we can declare a nested Enum:

'''

private enum SearchCondition {

SEARCH_BY_NAME.

SEARCH_BY_TEMPERATURE,

SEARCH_CONDITION_INVALID; // no condition or multiple conditions given

}

'''

If you would like to use 'int', maybe it would be better to make them static and change the variable names to indicate they are constants. E.g.

'''

private static final int SEARCH_BY_NAME = 1;

'''

Personally, I think Enum might be neater. 😄

Maybe we can add a space here? "TEMPERATURE RANGE " instead of "TEMPERATURRANGE".

This text will be displayed to the user, and the former may be easier to read?

Maybe one private method for each search condition? That would be great.

I'm not too sure but I think the execution will not reach this return statement? If that is the case, I think we can throw a CommandException instead of returning null? Returning null here may cause a NullPointerException somewhere else.

Perhaps we can rename this method to confirmSearchConditions or determineSearchCriteria? area may not be very clear that we are referring to the search conditions.

JavaDoc comments here are really useful to understand the purpose of the nested class. Great work 👍

@itssodium Could you check whether these changes are correct?

If I am not wrong, the Covigent part is the latest edit. It might have been overwritten by accident.

Would you be able to undo the deletion of this file? DateTimeUtil.java was recently added to manage dates and times.

Perhaps we could name this AddRoomCommand instead?

AddNumberOfRoomsCommand seems a little verbose since the number of rooms is implied, though this is a matter of preference.

If you like AddNumberOfRoomsCommand better, let's keep that.

Similarly, perhaps we could rename the command to addroom.

I think it's great that you refactored the code used for I/O. I/O should be abstracted if possible (SRP) since this class mainly handles the adding of the room.

Maybe we can follow the convention here? For messages, Address Book Level 3 follows this convention: MESSAGE_ERROR or MESSAGE_SUCCESS. We could use MESSAGE_NUMBER_OF_ROOMS_UNDEFINED or something along those lines here for consistency.

Would be good to add a comment here if the -1 means something.

Similarly, we can follow the conventions for messages here.

It is less about whether the user will enter a negative number, and more about how we should secure our application.

Personally, I feel that it is good if we can handle as many exceptional cases (sometimes called edge cases) as possible. Without proper handling, anyone can crash the app simply by entering a negative number. It can be annoying for the user if they accidentally type in a negative number and the app just stops working.

Besides, near the end of the semester, our application will be manually tested for bugs. I am fairly sure the testers will try to use negative numbers as inputs. If they can cause the application to crash, they will flag the problem as a bug and deduct our marks for it. Let's do our best to stop that from happening! 😄

May I ask what is the reason for naming the class RoomBook instead of RoomList or RoomQueue? I feel that Book does not explain what the class does.

Would it not be possible to use an ArrayList<Room>?

I think it is easier to understand if you iterate through all the Room objects in a loop and call an isEmpty() method to determine which Room is empty.

Performance-wise, I understand PriorityQueue may be better but we are sacrificing code clarity for performance.

Shouldn't Room contain an ArrayList<Task>?

@itssodium Would you be able to check this?

Do you mean it will be used for allocating the Patient to the Room?

Okay, leave this part to me. I will add in the TaskList.

Would it be possible to use Optional<Patient> here? While it is true that the Room can have no Patient, it is best to avoid null.

Great that you have a separate class for storage!

I think that is probably the temporary files used for testing? 🤣 Shall we rename the test files?

Yup, Address Book Level 3 has a convention for the names of test cases. Maybe we can follow that next time?

Ohh okay I follow your train of thoughts now. For adding Patient to Room, I think we should be more explicit? Maybe something like AddPatientToRoom.

I think you can use AddRoom for this.

May I check if URI is used here? I think path is a String and URI may not be needed here?

I think it would be good if you could leave a comment to explain the use of the nr and ro options!

Would it be possible to split this test into smaller units? It is currently longer than 30 lines.

Perhaps this part

'''

    rooms = new PriorityQueue<>();

    for (int i = 0; i < 10; i++) {

        Room room = new Room(i + 1);

        if (i % 2 == 0) {

            room.setOccupied(true);

        }

        rooms.add(room);

        roomsInArray[i] = room;

    }

    roomList = new RoomList(rooms, roomsInArray, 10);

    numberOfRooms = Paths.get("numberOfRooms");

    roomsOccupied = Paths.get("roomsOccupied");

    roomOccupancyStorage = new RoomOccupancyStorage(numberOfRooms, roomsOccupied);

    roomOccupancyStorage.saveNumberOfRooms(roomList, numberOfRooms);

    roomOccupancyStorage.saveOccupiedRooms(roomList, roomsOccupied);

    roomList1 = roomOccupancyStorage.readOnlyRoomOccupancy();

    assertEquals(roomList, roomList1);

'''

can be in a separate test?

I think addRooms here refer to allocating a Patient to a Room? Most likely, this test would be in a separate class so maybe we can remove it first?

Don't worry, I can handle this part. 😄 I will have to integrate the TaskList into Room to get my part working anyway.

I think it would be good to leave an explanation of why numberOfRooms is initialized to -1.

Perhaps the messages here can be moved to another class? Ideally, we can avoid adding constants to this class since the messages are stored in seedu.address.commons.core.Messages instead. Maybe look into putting your messages there too?

I think IntelliJ might have changed this to wildcard imports? Maybe we can revert this?

Do you think we should keep the indentation consistent? For *.fxml files, maybe we can use 2 or 4 spaces (not sure what the original is).

I think it would be patients here. Very minor grammatical issue though 😄

I like how there is an order for the different types of entities (patients followed by rooms followed by miscellaneous). Maybe in the future, we can a header to each entity to further organize the content of our user guide? E.g.

'''

3.1 Command Format

3.2 Patients

 3.2.1 [Add a patient: 'addpatient'](#32-add-a-patient-addpatient)<br cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf">

// ...

'''

Do you think we should standardize this? Not sure if it is as important in the JavaDocs because it is understandable, but what about the user guide?

Should we refer to the application as the app, the application, or Covigent?

Thank you for noticing and changing this! 😄

Because isSamePatient(...) uses name, age, and phone number as criteria to detect duplicate Patient in a list, will this not pose a problem when deleting? It is possible to have more than 1 Patient with the same name after all. In that case, only the first matching Patient is deleted.

I think it may be a little strange that the user cannot select which Patient to delete if they share the same name. I am not sure about the optimal way to solve this problem. Would deleting by index be better?

Good job noticing the bug here! 😄

Maybe this can be renamed to indicate that an invalid search criterion has been provided. searchPatient does not really explain which aspect is being tested.

Something like execute_searchPatientInvalidSearchCriteria_throwsCommandException() may be good?

I think it would be good to mention something along the lines of how the command replaces the current number of rooms with the newly specified value. Also, whether the data in the current rooms will be migrated or reset.

We can always improve on the user guide in the future though, so maybe we can KIV? 😄

Since there are multiple files being checked, maybe we can include which data file is not in the correct format in the warning.

The same goes for the other warnings.

Maybe we can keep this statement in a single line such that it looks even neater? It has fewer than 110 characters and can fit in one line!

Perhaps we can remove code that is commented out. If we merge this, we may forget to remove it in the future. Also, commented out code seems like regular comments and may be difficult to locate in a large codebase.

Great work with the hashing!

May I ask why the 31 is needed here?

I think maybe we can make this more general and apply it all the other *List classes (UniquePatientList, RoomList, TaskList). Otherwise, I think the interface may not be needed?

Possibly ReadOnlyList<T>? We can do either:

Include all methods such as add, delete, etc. (much more work because we have to rename all methods to share a common name).
Include just the method to obtain a read-only list (easier).

Maybe we can add a TODO here to indicate that in the future, we should set the application default to having a few rooms with patients and tasks?

Although ObservableList and PriorityQueue maintain the same rooms, for safety would it be better to include a comparison for the ObservableList too?

The condition for this is rooms != null && roomList.rooms != null but I think neither the PriorityQueue nor ObservableList can be null because of the default value, right?

In that case, can you not just return false if any of the 2 PriorityQueue and 2 ObservableList are null?

Instead of tracking numOfRooms in a variable, would it be possible to just make use of the size of the ObservableList? Maybe include a method to get size (similar to the other *List classes).

This would reduce the complexity of the class, especially when writing the equals(...) or hashCode() method since there is one fewer variable to manage.

'@JsonProperty' tells the Jackson ObjectMapper to map the JSON property name to the annotated Java field's name so we end up with this:

'''

"room": {

"roomNumber": value

"isOccupied": value

 //...

'''

For consistency with 'JsonAdaptedPerson', perhaps we can keep it the same as the variable name (i.e. roomNumber and isOccupied)? We would have

'''

public JsonAdaptedRoom(@JsonProperty("roomNumber") int roomNumber,

    @JsonProperty("isOccupied") boolean isOccupied) {

//...

'''

We can add a TODO here since we are not using the Patient and Task in this class yet.

Consistent @JsonProperty here! Variable name should be rooms.

Good job with switching to temp folders! 😊

Maybe we can import java.util.Optional directly?

I think if we are not using this class currently, we can remove it 1st?

I will add this when I pull your changes 👍

Might be good to declare a constant for "-" to indicate what it is doing more clearly!

Might be difficult to switch here since there are multiple conditions.

I think the current structure is not too bad. Maybe we can rename clearRoom to isNewEmptyRoom for clarity? clearRoom sounds like the name of a method hahaha.

Also, I think isCurrentlyOccupied sounds more like a boolean. How about we use that instead of alreadyOccupied?

Instead of ...orElse(null) != null, maybe we can use editRoomDescriptor.getIsOccupied().isPresent()?

Would it be possible to combine case 1 and case 3 since both return Room(updatedRoomNumber)? 🤔

!hasNewPatient could be the condition.

Actually, I'm curious about why clearRoom is needed as a condition.

While not entirely linked to this PR, I think we should modify Room to store an Optional<Patient>. Then, we can remove the isOccupied boolean and use Optional<Patient>.isPresent() to find out whether a room is occupied.

I understand it may be easier to return an Integer but this method is similar to the parseIndex. We can use the Index class provided in the original codebase and use parseIndex instead.

That was what I did in AddTaskCommandParser 😆

Hmm if we modify RoomList directly from EditRoomCommand, we may be breaking a few abstraction barriers since Command will be interacting with RoomList.

To avoid this problem, we can add a setRoom(Room target, Room editedRoom) method to RoomList. Then, add another setRoom(Room target, Room editedRoom) method to Model (the interface). Finally, In ModelManager, call roomList.setRoom(...).

We can then use the setRoom(...) method by calling model.setRoom(...). So we are restricting the interactions between classes such that Command only interacts with RoomList via Model.

Does DuplicateRoom apply only to identical RoomNumber?

Thanks for adding this TODO! 👍

Wait, I see that setRoom has been done. Good job! In that case, why is the modifiable room list still needed?

Also, perhaps we can rename these methods to hasRoom(...) and setRoom(...) for consistency with Patient?

Okay, I see what you mean. For now, we are treating RoomNumber as an index but in the future we may allow the user to set their own room numbers.

Would it be convenient for you to move the room-related methods to RoomParserUtil?

If not, I could do that later because I intend to make ParserUtil an abstract class.

getRoomList() returns the ObservableList though. Wouldn't it be possible to check if the room is present from there?

I think one reason we would not want to return the RoomList is because it is mutable. That means that if we return RoomList from ModelManager, any other class can modify the content through RoomList (e.g. call roomList.addRooms(...)) instead of through the Model (i.e. call model.addRooms(...)).

getRoomList() returns a read-only list so we can prevent the contents of RoomList from being changed directly. In fact, I think that was the original intention behind the asUnmodifiableObservableList() method.

Okay, noted on this. I will make the change. Thanks!

Agreed! Let's remove the additional fields.

Yes, right you are! I will change this.

Interestingly, yyyyMd does not work.

Consider the date "12 Jan 2020", for instance. With yyyyMd, it would be "2020112". But this string is ambiguous. Are we referring to "2 Nov 2020" or "12 Jan 2020"?

If we want to use yyyyMd, we would need a delimiter (e.g. yyyy-M-d). I decided against using the hyphen because it doesn't read as well when it is a date range.

Actually, yeah. It is probably not needed for an MVP.

The rationale behind adding DateTimeCreated is to allow the user to filter tasks with creation date as a criterion. For example, in Slack, users can see when the task is created. It is helpful when the user wants to query what tasks were created yesterday or within the last few days.

Right now, the plan is for the user to click on a Room in order to view all tasks assigned to it. There is no separate tab for the users to manage tasks (we should probably have this to show user which tasks from which rooms are due?). In the future, when we implement reminders or a search for tasks, DateTimeCreated might be more useful.

I think I will remove this from the MVP. Unit testing for this class also requires more work because we would need another layer of abstraction to handle and control the date-time instead of using LocalDateTime.now().

Thoughts on this?

Okay!

Nope, it is in an entirely different file. That said, I wonder if it will be confusing for the user?

No problem!

I think p/ is good 😄 We can try to keep it as short and sweet as possible since that's easier to remember.

Good point. Will use that instead 👍

It is missing the success case actually. Will need to set up a stub to perform the test. Will KIV 1st.

Hmm could you clarify on this? I think most of the other Command classes handle add, delete, and update through Model. The Model is the abstraction.

@itssodium would you like to close this PR? I think we could keep the other one instead?

Good work! We will have to correct the CheckStyle problems in order to merge. Would you be able to change the order of imports?

We have decided to close this issue. Don't worry about creating the pull request from master.

For future pull requests, remember that every new feature should be done in a separate branch!

Task models are added 1st. Will be integrated with Room in v1.2.

While not ideal, I decided to force push to my own repo in order to squash the commits that contained only minor changes for CheckStyle compliance.

I will be more careful when pushing to my repo in the future.

Overall, clean and neat code! Believe you haven't integrated the command into the code yet but seems like good support for your command for now. One thing though, I am not sure why you want to have a DateTimeCreated for task? Anyway, LGTM!

Haha, yeah I set up the PR to update everyone with my progress. The integration is not complete yet. I will request for more reviews when the integration is done.

To fix your addressbook and let it show its contacts, follow these steps:

Go to your data folder

Open up addressbook.json

In the second line, change "persons" to "patients"

I recommend deleting data/addressbook.json if you have not added any custom data. Upon restarting the app, the file should be regenerated.

Additionally, the logic in 'readAndSaveRoomList' unit tests may be difficult to follow without comments. Do you think it would be possible to add

'''

// Save in new file and read back

RoomOccupancyStorage roomOccupancyStorage = new RoomOccupancyStorage(numberOfRooms, roomsOccupied);

// ...

'''

to indicate that you are saving and loading the file for testing?

There is also some code duplication in the 'readAndSaveRoomList' tests, which we can consider refactoring into private methods.

I removed the link to #58 because we haven't actually displayed the Patient and Task details for each room. We can definitely merge 1st though.

Rebased to avoid additional merge commit.

lgtm, good job! However, the pr is super long, would it be better to like split it up in the future? eg. do the tasklist/task class and merge it first. then the addTask command and then merge it?

Totally agree with this. Ideally, I think a PR should not exceed 500 lines. It is difficult to review the PR when it is that long. I originally wanted to split this as well. Will definitely do so in the future! 👍

New feature added does not correspond to the feature you were assigned. Possible typo?

Examples provided not changed to match the changed feature.

User command should be just "list" instead of "list 2" to fit the explanation fo the command. Possible typo?

Maybe changing it to "displays all your assignments and lessons for the next 2 weeks" would be more succinct?

How about "displays all your lessons and assignments for the next 2 weeks (including the current week)"?

Looks good!

@printinghelloworld sounds good 😃

Looks good 😃 A minor nitpick is that you may want to consider removing point number 3 ("ASM retrieves lesson data from NUSMods API") as according to the textbook, use cases only describe the externally visible behavior, not internal details, of a system.

Similar to the above comment, maybe it would be better to change the phrasing to describe how the ASM displays the lessons from the newly imported timetable

Maybe including the time the assignment is due in the deadline would be better?

E.g: n/Lab report 3 d/23/04/2020 1230 mod/CS2100

Since we have some optional fields as well, maybe it would be clearer to the user if we changed the description given in AB3 to match our project?

I was suggesting to keep the Tip section and change the description to "You can use the remind tag to set reminders for the assignment you want to add instead of using the remind command after adding the assignment" or something along those lines.

Currently, module code can accept the same string format address in AB3. Perhaps consider changing it the VALIDATION_REGEX to match the format of module codes as well?

Testing of new ModuleCode field may be more accurate if the input was changed to match the module code format.

INVALID_ADDRESS_DESC should be INVALID_MODULE_CODE_DESC for consistency 😃

Similar to the above comment, VALID_ADDRESS_AMY should be VALID_MODULE_CODE_AMY.

Similar to VALID_ADDRESS_AMY comment.

Name of method still refers to address instead of moduleCode. Possible typo?

Could consider changing the test cases to test for valid module codes 😃

According to the 2103T textbook, the suggested guideline for code quality is to break-line after the bracket instead of in the middle of an instantiation.

E.g:

JsonAdaptedPerson person = new JsonAdaptedPerson(

VALID_NAME, VALID_PHONE, VALID_EMAIL, null, VALID_TAGS);

The comment describing the test still refers to address field instead of module code field.

Similar to other comments, the comment describing the test still contains address.

Comments still refer to address. But it is just a minor nitpick 😃

Similar to other comments 😃

This line can be removed 😃

Comment still refers to address instead of module code

Maybe could do more testing to ensure that the regex works as it's supposed to? You can consider testing for modules codes with no alphabets at the start ("1010S"), modules with no numbers ("GER"), modules that begin with more than 3 alphabets ("ABCD1000") etc

Comment still refers to address field.

Similar to other comments relating to address comments 😃

The comment describing the email test is not deleted.

The comment for this test still contains 'email'

yup

This test case seems to still have references to email.

Comment still references email.

Comments for this field use 'persons' instead of 'assignments'. Be sure to check the comments as well 😃

Similar to the other comment, the comments for the field use 'persons' instead of 'assignments'

Similar to the other comment, the comments for the method use 'persons' instead of 'assignments'. Same for the equals method below this method 😃

Similar to the other comments, the comments use 'persons' instead of 'assignments'

Similar to the other comment, the comments for the method and the string parameter at line 9 use 'persons' instead of 'assignments'.

Similar to the other comments, the comments use 'persons' instead of 'assignments'.

Similar to the other comments, the comments use 'persons' instead of 'assignments'

Nice test cases 😃 You may want to consider leaving a space between some parts of the code to make it more readable (e.g leaving a space between lines 79 and 80 and leaving another space between lines 81 and 82)

Similar to the previous comment on improving readability 😃

Similar to the other comments, the comments use 'persons' instead of 'assignments'

According to the CS2103T textbook, the convention for the indentation for wrapped lines should be 8 spaces.

Following the CS2103T convention, you may want to consider changing the indentation to match the one in red 😃

You may want to consider leaving a blank line between some lines to improve readability and code quality (E.g leaving a space between lines 82-83 and lines 84-84). You can refer to the screenshot of the CS2103T coding conventions below 😃

Similar to the comment above on readability 😃 You may want to consider applying the same thing to the other test cases in this class

JavaDocs should be @moduleCode instead of @address 😃

JavaDocs should be @ModuleCode instead of @Address

Since assignment starts with a vowel, it should be "an assignment" instead of "a assignment" 😃 The same typo can be found throughout the project. You can consider using the "find in path" feature in IntelliJ to help you find them (Edit -> Find -> Find in Path)

This test case should be removed 😃

May want to consider adding a link back to the main point which is that ProductiveNUS is supposed to help NUS Computing students resolve the problem so that it is clear what the aim is 😃

I also think it may be better to use more passive language? So instead of 'You often...' it may be better to use 'You may often...'. What do you think?

Looks good! A suggestion could be to bold/box certain words to make the aim more obvious.

The test cases look good! Minor nitpick, the comments next to the valid test cases do not match what is being tested. Also, if the lesson names do not accept numerals in the name, you may want to consider changing the validation regex to only accept alphabets 😃

Nice test case 😃

okie

Naming the deadline field time may be more appropriate for the Lesson class as it doesn't really make sense for a Lesson to have a deadline.

Personally, I feel that using 'schedule' is a little vague. Maybe you could consider changing it to 'schedule manager' or 'schedule book'? What do you think?

Message for duplicate assignment still refers to address book.

According to the CS2103T code convention, boolean variables should be named to sound like booleans (e.g isRemindTypo/containsTypo instead of remindTypo).

I think can just use the hashcode method you created in Lesson.java as given the way you implemented it seems similar to how AB3 implemented it for Person, I think lessons.hashCode() should be fine.

According to the CS2103T coding standards/conventions, we should try to avoid deeply nested methods. Maybe you could consider removing the else block and extracting the "happy path" to prevent the method from becoming too deeply nested. Another suggestion is adding an empty line in between lines of code to improve readability. You could consider applying the SLAP framework as well 😃

I'm not very sure what "Lists the tasks within the next index number of weeks" means. Maybe consider changing the explanation to what is stated in the User Guide?

Nice! You could consider abstracting out the implementation as a private method as the execute(Model model) method is a little long now and according to the CS2103T conventions on code quality, we should try to avoid too long methods 😃

According to the CS2103T textbook, under code quality, we should strive to extract the "happy path" out. You could consider changing the code to extract the "happy path" 😄

e.g:

if (!isNotListAll) {

return new ListCommand(Index,fromZeroBased(0));

}

try {

....

}

According to the CS2103T coding conventions, we should delete code that we do not need 😃 Are you planning to use the commented out code at a later date?

JavaDocs still refer to assignments instead of lessons

Similar to the comment above regarding the JavaDocs 😃

Similar to the comment above regarding the JavaDocs

String still refers to Assignment instead of task 😃

JavaDocs still refer to assignment instead of Task

Similar to the above comment regarding the JavaDocs.

JavaDocs still refer to assignments instead of tasks

JavaDocs still refer to Assignment instead of Lesson

Similar to the comment above regarding JavaDocs

JavaDocs still refer to Assignment instead of Task

Similar to the above comment regarding JavaDocs

Good job 😃 You may want to consider abstracting lines 125 to 135 into another method as this method is quite deeply nested. Remember to SLAP hard 😄

address name should change to module code after merging the PR that changed the address field to module code

Sounds good!

Amended 😃

Since we are probably going to be adding an edit feature in the future we can just leave it in first or add it to v1.2 since the implementation is all done for us already 😄

What do you mean?

LGTM.

Will description 'Tags your assignment with a reminder which adds it to 'Your reminders' section' be clearer?

Noted! I have edited the phrasing. Is the edited version clearer?

I have changed some of these user stories to make it more specific. eg. 'work'

Perhaps refer to DG to see which is better?

Updated accordingly 😄

Looks good! Just a small suggestion, maybe we can standardise the use of capital letters under 'I can' and 'so that I can'. we can either capitalise the first word or not capitalise the first word for both scenarios 😃

Sounds good! Which do you suggest is better? I'm fine with either way 😃

Looks good! Just a small suggestion, maybe we can standardise the use of capital letters under 'I can' and 'so that I can'. we can either capitalise the first word or not capitalise the first word for both scenarios 😃

Sounds good! Which do you suggest is better? I'm fine with either way 😃

I think for now we can not capitalise for both scenarios !

Amended 😃

Yup, we should standardize comments. I think we can just follow the style you suggested. Everyone okay? @raymondge @itssodium @chiamyunqing

is it be better to use comment.trim().isEmpty() instead of comparing it to null?

i think you can use the TODO function so its easier to check what you have to do next time

eg.

//TODO: your todo item here

i'm not very sure, does the java doc require the parameter and the return value? eg.

'''

/**

Returns true if a given string is a valid age.
@param test....
@return boolean.....

**/

'''

same for the java docs here!

okay can!

is it better if we do addpatient instead of addPatient? just asking out of curiousity, no need to change if you think its fine

i think this should be addPatient instead of add?

do you think we should add in details like the temp must be 1dp or that the age must be in the range from 0-120?

hmm, why do you need this variable? Essentially, the numofRoomsInString is equal to Integer.toString(numOfRooms). so maybe you can just use that instead of declaring another variable?

so for this, you can just use Integer.toString(numOfRooms)

maybe you want to change the name to SearchPatientDescriptor? can rightclick, refactor -> rename

not sure if this will fail the checkstyle because of multiple empty lines

i think you can change the Person's to Patient's

i think that this method might be a bit long, so maybe you want to extract out some lines of code into a new method? Its just a suggestion, it's fine without it also

I think the one that was removed is correct? Might need to check

is there a reason behind initializing the numerOfRooms to -1?

perhaps it might be good to catch the exception for negative integers inputted by the user?

Sorry, I don't really understand the need for a PriorityQueue in this class, can you explain the rationale for ti?

can i know whats the purpose of this constructor? because its empty but yet it takes in an integer parameter

i think i saw the method below this comment in the RoomBook class, so there might not be a need for this method here?

same as the previous comment, this method might be a duplicate?

since you are creating a new room every time the user adds a room, i think the previous data in the room might all be wiped?

this might be a duplicate method too?

it's only null for now right?

i think this might override the data of the previous rooms too?

i think you might want to change your naming for the priority queue and not use rooms? i got quite confused trying to read it

since you are only counting the number of occupied rooms directly, will there be a problem with this? eg.

i have 3 rooms

room 1
room 2
room 3

So lets say room 1 and room 3 are occupied. so numberOfRoomsOccupied = 2 correct? But next time if you load the RoomBook, it will give me room 1 and room 2 occupied since the numberOfRoomsOccupied is 2?

is the name of the file lol?

this indentation might be abit off?

let's work with the naming together for the test cases next time!

is there a purpose for this method?

i think you can delete this method if it is not in use

hmm, i dont really think there is a need to test if the priority queues are equal? or if you want to test, you can just do

'''

int[] array1 = roombook1.toArray();

int[] array2 = roombook2.toArray();

Arrays.equal(array1, array2);

'''

i think better naming of the paths would be good? it's a little bit hard to understand/remember what the constructor of RoomOccupancyStorage is supposed to take in

by accident? maybe a slip of the finger?

hmm, but i think you can just chain the methods together? like .getRoomBook().getRooms() etc

hmm, i thought that this field not supposed to be here anymore?

Hopefully we won't get merge conflict for this when we merge our UI hahaha

thanks for helping me to change what i have missed!

same here, thanks for catching this

should we make it such that there cannot be patient with the same name?

Is there a typo here? don't think we have UndoCommand and RedoCommand right? hahha

if this part is not needed, then will it be better to just delete it?

i think you can delete this line and subsequently, the convertPriorityQueue method

can delete the method below. i dont think it's in use anymore. i only created it to bypass the need for an observablelist

i'm not sure why it cannot be yyyyMd, is there a difference?

i think he used it because it allows easier tracking of the tasks due date using DateTime instead of a string

Very good use of ObservableList, this will help immensely in the integration into the UI!

i think this should be null dateTime instead of email?

i think this prefix is already used for periodOfStay in PatientCliSyntax.java, will it be affected?

thanks for cleaning up my unused method from before! 👍

i think its fine. It makes me wonder if I should convert my editroom prefixes to p/ instead of pn/ for patient name. hahahah

Yup, adhering to abstraction principle is good, however i feel it might not be worth it to extract only 2 lines of code into a new method. maybe what wl can do is to perhaps give a name to the variable to make it clearer.

eg. String[] stringNames = searchPatientDescriptor.getStringName().split("\\s+"));.

same here, abstraction principle is good, however, it might not be worth it to extract them out into a new method

yup! is there a reason why you are checking if the search result has only has one value? I'm referencing to the (patientNameList.size() == 1) cuz your getListOutput(list) method will just print out everything

hmm, just curious ah, why dont want just String stringName = argMultimap.getPreamble().trim().toLowerCase()? so eg. if you have searchpatient alex n/something, your stringName will be alex.

Also, what is the substring(2) for ah? i think for very long but i still cant figure it out

can consider Returns instead of Return? also, do we have to leave a line after line 70? i kind of remembered that it is in the coding convention but im not sure.

might be good to have a java docs header before this!

maybe you can use model.getUnmodifiableRoomList() here? model.getUnmodifidableRoomList() returns an ObservableList too. i discussed with yh last time about how the command class shouldnt be interacting directly with the room class but rather through the model class

i changed the checkIfRoomPresent() method to a static method because of the reason below. ideally, i feel that checkIfRoomPresent() should be in RoomList.java class. what do you think?

should there be an empty line before this line of code in the java docs?

should there be an empty line before this? also, there are some extra white spaces in line 23

same for the empty line before this

ah, i see i see, sorry!! hahaha

do you want to consider removing this comment? is it needed?

would be good to have a class header for java docs!

you can consider renaming your test method to toModelType_returnsOccupiedRoom_success() here.

same here for the naming

i think you can consider looking at the JsonAdaptedPersonTest class for an example of what to test!

hmm, why do you need this method? is it to get access to the ObservableList? If you are not planning to modify the elements in the ObservableList but planning to "look at" the elements in it, you can consider the asUnmodifiableObservableList() method in this class at line 84

do you want to consider moving these few lines of code into the map function from line 55? you can just write a one liner val = trimmedDueAt and i think it should do the trick?

can consider val = trimmedDueAt so that any whitespaces are removed

let us know if you need help with writing json test cases! Although Json is unfamiliar to me too, i'm sure we can work something out

consider adding a java docs header here

consider adding a java docs header here. Looks good to me for the testcases!

you might want to standardize the naming of methods, so maybe this method can be toModelType_sameModelType_success(). same for subsequent methods below toModelType_sameModelType_throwsException(), toModelType_sameModelType_failure()

remember to remove println statment here. you can consider adding a //TODO before using println statements, so you won't forget to remove them

makes sense, let me change it later

oh ya, i forgot. ill change it later

okay, let me change it now and update the pr

yup, i think i missed out on that, thanks!

i thought its already editpatient?

okay can can

whoops, let me rectify that

the - means that its deleted

all the red lines are deleted lines!

okay, will change it, thanks!

okay can, I will change it

i think instead of arraylist, it might be better to use ObservableList directly. ObservableList is what is passed into ListView (UI component) and allows the UI to be updated whenever there is a change in ObservableList.

okayy, ill change that real quick

yup, i will change the indentation. good catch

alright man, i have reverted all the wildcards imports! thanks for pointing it out

the "-" is to remove the patient from the room! so eg. editroom 1 pn/-, it will remove the patient from room 1

okayy, i will change it later! thanks for pointing it out

yup, good idea

i'm not sure how to use a switch here because the if conditions are all different, maybe you want to give me some suggestions?

yup sure! i will change it

cuz i feel like its not really an index, so grouping it under the index class seems wrong

okay, let me try it out

yes

because i have to check if the room is present in the ObservableList. There is no way to access the RoomList other than through the ModelManager.java. i don't think its suitable to have a method in ModelManager.java to check if the room exists and to return that room

sure, i can move the room related methods to RoomParserUtil

I'm allowing the users to set the room number already, so index might not be that great...

yup, i just changed it!

i initially used hasRoom() and setRoom(). i forgot why i didnt use hasRoom() but i didnt use setRoom() because roomList class already has setRooms(), i feel that it will be quite confusing to have 2 methods named so closely to each other but does totally different things

Yup, my test cases were failing, so I added a toString() to check. I thought about removing it, but I figured you might need it. Feel free to delete later. Don't think it matters

Hmm, now thinking about it, it does seem like bad design. I'll change this later and update this pr. I will probably use the observablelist.stream().anyMatch to replace this (so it will stay in EditRoomCommand.java but won't have a method) . Do you think its a good idea?

This is one of the dilemma I'm facing. Whether to have a static method in RoomList.java or have a method in EditRoomCommand.java (that is out of place). I think i will adopt the same method of just using the observablelist.stream.anyMatch() method and not create a new method. How does it sound?

Since i have to change other parts, I think i will just update it in case I forget about it in the future

thats true, let me shift the code there then

i will shift this method to the model manager also

okay sure!

HAHAHAH, i wrote it that way so the method name not so long. will change it later!

good catch on the ambiguity! i will make it clearer!

there should only be one patient in a single room though. also, it doesnt move the patient, you have to clear the patient from the room first. maybe for v1.3, i can implement the auto remove and move feature

I'm not sure if <ins> is the correct tag to use here? (And the closing tag should be </ins> right) From what I gathered online <ins> is to underline some text in html to indicate it is an insertion, but there's not <del> tag here, so I think it might be more appropriate to use either the  tag for underlining or []() syntax if its supposed to be a link

I think we should retain the step of copying our jar file over to the folder the user wants to use as the home folder, as we'll need to do saving in a data folder as well

There's inconsistency between this and the description of the help command below. Also, I think the help contents will be opened in a window and not a page?

Refer to comment above

Maybe we should change PHONE to PHONE_NUMBER, following the original format, for clarity

'''suggestion

Shows commonly used commands for TBM.

'''

'''suggestion

Role: In charge of deliverables and deadlines

'''

Following the format for client delete INDEX, maybe add a short one line statement to explain what this command does?

Same as prev comment, and there's also an extra newline here that I think would be better to remove

The indexing is off here

Indexing off here too

'''suggestion

1a. The list of clients is empty.

'''

Might want to change this to match the original "Finds clients whose names contain any of the given keywords.", and change n/NAME to KEYWORD [MORE_KEYWORDS] as the format section below mentions keywords

'''suggestion

The search is case-insensitive. e.g 'hans' will match 'Hans'
The order of the keywords does not matter. e.g. 'Hans Bo' will match 'Bo Hans'
Only the name and country are searched.
Only full words will be matched e.g. 'Han' will not match 'Hans'
Persons matching at least one keyword will be returned (i.e. OR search). e.g. 'Hans Bo Russia' will return 'Hans Gruber', 'Bo Yang', 'Alice Katya'

'''

'''suggestion

Listing all clients: 'list'

Shows a list of all clients in the address book.

'''

'''suggestion

Finds clients whose names contain any of the given keywords, or whose country of residence contains any of the given keywords.

'''

I think we should just stick to either the abbreviation TBM or the full name in the headers, not both

either that or perhaps change both back to the simple "User Guide" and "Developer Guide". I don't really see a need to have TBM in the title, as the whole website is already about TBM, plus we have introductions and our home page.

Just to add on, I don't think the <ins\ cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> tag should be used here, as markup defines it as inserting something? e.g. here. <u\ cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> seems more appropriate if the intention is to underline the text. </ins>

LGTM

'''suggestion

    this.countryName = new Locale("", countryCode).getDisplayCountry();

'''

I think its more intuitive to call this specific method

'''suggestion

private final String COUNTRY_NAME;

private final String COUNTRY_CODE;

private final ArrayList<note cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> COUNTRY_NOTES;

'''

From the convention guide: Constant names must be all uppercase using underscore to separate words. I'm not sure if countryNotes should be marked 'final' actually, since we are going to modify it in the 'addCountryNote' function.

Also, I feel that the notes should be stored in something like a LinkedHashSet, since we don't really want to add in duplicate notes, and its easier to check if its a HashSet.

I feel like we shouldn't initialize countryNotes until it is actually needed. E.g. Only when the user actually adds a country note, or there are notes for this country in the save file. If not its unnecessary overhead, especially if there are many countries.

Not sure how the rest think @tankangliang @rtshkmr @LeeEnHao

Constant names must be all uppercase using underscore to separate words.

Is there a need for this Map? From what I see, I think we can just keep the ISOCountry String array, then check if the entered String is inside, before initializing a new Locale with that String. This way, we won't have to keep so many country objects, since most of the countries likely won't be used.

Constant names must be all uppercase using underscore to separate words.

Ah ok nvm I think can just stick with this for now. I was thinking of something else to prevent the unnecessary creation of objects, but this HashMap is still necessary for it, my bad.

Btw can change the HashMap to Map, so that we standardize

Hmmmm seems like by constant they mean static final only. Static only or final only aren't counted as constants, even though one of the examples in the website has an example of final only variables being set to all caps + underscores.

No need to change bah

Would be better to standardize with the other commands, for example the ClientFindCommand.

Btw, ClientFindCommand doesnt have requireNonNull(predicate), I think can just add it in there too

There needs to be a javadoc comment here. (All public classes should have javadoc according to the textbook)

javadoc comment

requireNonNull

Might want to standardize with other equals methods. Basically these 3 parts: short circuit if same object, instanceof handles nulls, and state check. Can look at ClientEditCommand for an example

Maybe can add extracting out the messages into a static final string into the TODO: better messages, in case we forget

javadoc comment

Might want to add a TODO here

This whitespace is little bit weird lol, but might just be my personal preference

here too

Might want to change the predicate name to ClientCountryMatchesInputCountryPredicate for clarity?

might wanna change this equals to match the 3 step thing

Probably should calculate and return the hashcode using noteContents, since I think Note is already being used hashed objects

Would suggest changing this to isValidCountryCode, for clarity

Hmmm I was thinking that this CountryManager class holds all Country objects, while every other class should be holding references to these objects instead of any new Country() object. That way, the class themselves should be able to check if the Country has any notes, and this method would not be needed.

Can help to update the parser names for the delete and find command?

Should probably add in a hashCode() function just in case

Actually for this toString, should we be returning countryName instead? Maybe can add a comment to address why we're returning countryCode.

The whitespace is off

Add a comment for standardization

'''suggestion

    // multiple countries - last country accepted

'''

not sure about that, but later that guy forget lmao

I think we should just do it now man. From java docs: Note that it is generally necessary to override the hashCode method whenever this method is overridden, so as to maintain the general contract for the hashCode method, which states that equal objects must have equal hash codes.

Yeah, but usually the purpose of the toString is to return a more "human readable" format of the object, so maybe countryName would be a better choice for the return value.

LOL ok then just fix it in that issue bah

Would be slightly clearer if you changed the variables names to SMALLEST_NEGATIVE_OFFSET and LARGEST_POSITIVE_OFFSET imo

Not sure why this sys print is here

Add new tests for timezone pls

We should catch NumberFormatException here, in case people enter a number that is longer than what an int can hold

Extra new lines, and I think the javadoc can be added already right

Would be better if MESSAGE_USAGE is moved upwards for organization purpose, so that public static final is above private static final

Also, its hard to read when the lines are separated so much with the random comment also, would suggest this:

(must be a positive integer should also be moved to the description, not inside the parameters)

Rmb to import PREFIX_NOTE

'''suggestion

        + "Parameters: INDEX " + PREFIX_NOTE + "NOTE_STRING\n"

        + "Example: " + COMMAND_WORD + " 1 " + "client note content";

'''

I think everything has to go through model, as it is being used for UI I think

This variable doesn't seem to be used, since you already removed the redundant code

agree with rayson

Would be good to remove the line break for readability

Would be good to move this down, after the if statement

Probably would be good to add a javadoc comment for this method as well

There is a need to implement the hashCode function, since note is being used in a hashSet.

Could you also change the equals method to match the 3 step thing? (1. short circuit if same object. 2. instanceof handles nulls 3. state check)

There's like extra newlines here and above as well

Extra newline

I would suggest that this is changed to a LinkedHashSet, as we will want the order of the notes to remain the same as the order they were added. HashSet does not guarantee order

I believe the country notes are also using LinkedHashSet

Also, I think it would be better if the clientNotes was initialized only if the client has notes, probably can add a TODO here to optimize this.

I think this variable still isn't being used

Other than that, LGTM

'''suggestion

public String getInputCountryStringRepresentation() {

'''

Better to have descriptive function names without abbreviations imo

Probably can be on the same line for clarity

Would be better to move this above MESSAGE_DUPLICATE_COUNTRY_NOTE so that the public static final variables are tgt.

LGTM

I think "the filename of the profile photo should be doc/images/githbub_username_in_lower_case.png" - Week 7 Point 2 in Project

Might wanna consider team/samlsm instead

Missing space before 4a.

Remove extra space

Missing "Use case ends."

Inconsistent comma, (+ EMAIL_DESC_BOB,)

//// student-related tests may work better here instead of Student, since we also have //// moduleClass-related tests

Extra space

See if its possible to trim down this part. I took a couple of tries before I understood what was going on

The {@code ModuleClass} must not exist in the current list

Don't quite understand this. Specifically, don't understand what is a backing list and what is meant by unmodifiable

'''suggestion

/** Returns an unmodifiable view of the filtered 

 * {@code ModuleClass} list.

 */

'''

'''suggestion

// student-related constants

'''

'''suggestion

// moduleClass-related constants

'''

Need full stop for this comment

Comments start with lower letter + no full stops

'''suggestion

    // workaround as storage functionality for ModuleClasses has not been implemented

'''

Ok lets retain it in that case

Missing space

'''suggestion

        + PREFIX_TELEGRAM + "johnDO3 "

'''

Consider keeping the previous tag set? Because its more of an app to track students rather than keep address contacts.

I think the tagset got overwritten. Saw that you merged the master branch into your branch

Might be good if we remove the telegram prefix, since user has to type @ for email. So we might want to keep the design consistent by making them type @ for telegram handle

An alternative would be to update both the UG and DG to reflect the telegram handle changes.

I personally would go for the former than latter. How about the rest?

Any reasons why "student" was removed?

Extra white space

Any reasons for removing this white space?

Resolved by sticking to using TELEGRAM_PREFIX = "@"

Any reasons for removing this white space?

Resolved by adding back "student" in command

'''suggestion

 * @param index Index of the class in the filtered class list to edit.

'''

'''suggestion

 * @param editModuleClassDescriptor Details to edit the class with.

'''

'''suggestion

    // short circuit if same object

'''

'''suggestion

    // instanceof handles nulls

'''

'''suggestion

    // state check

'''

'''suggestion

 * @throws ParseException If the user input does not conform the expected format.

'''

I think the full stop got removed for some reasons.

My current updated code base has a full stop for this comment

'''suggestion

Parses input arguments and creates a new FindStudentCommand object.

'''

@throws ParseException If the user input does not conform the expected format.

'''suggestion

        || (other instanceof ModuleNameContainsKeywordsPredicate // handles null)

'''

start with lower case, e.g

'''suggestion

    // one keyword

'''

'''suggestion

 * Returns an {@code EditModuleClassDescriptor} with fields containing {@code moduleClass}'s details.

'''

Can you help center the fencing for the table?

Can you help organise/center the fencing "|" for the userguide table as well? I remembered centering it awhile back during v1.1 but seems like some of the fences got out of position again

'''suggestion

Shows a list of all students and all classes in the application.

'''

Might want to consider putting this after DeleteModuleClassCommand.COMMAND_WORD but before LinkCommand.COMMAND_WORD to maintain a fixed action sequence

"3" seems like a magic number across the test script. We might want to consider abstracting it into a variable in a separate PR

I think we should use Color since the naming before the change was Color

I agree with Dexter on this

'''suggestion

 * Deletes the given {@code Student} and the {@code Student}'s {@code UUID} in all {@code ModuleClass}es.

'''

'''suggestion

'''

'''suggestion

'''

'''suggestion

 *  Deletes {@code key} from this {@code TutorsPet} and also the {@code UUID} of {@code Student} from all {@code ModuleClass}es.

'''

Just to clarify, the reason why you create a new hashset is because the set returned by getStudentUuids() is immutable?

'''suggestion

    // manually remove UUID

'''

Is this change because if a student is deleted, then the associated student in ModuleClass is removed as well?

Extra space

Can we remove this space? Since line 17 and 19 are both messages

Space between line 15 and 16?

The words undone and redone are kinda awkward but I think its okay if we express it in that way, since it still gets the message across in a succinct manner

'''suggestion

 * Creates a {@code StateRecords} with the specified {@code currentIndex} and {@code stateRecords}.

'''

Extra space

Resolved by removing MODULE_CODE

Resolved by removing [] for all fields within add apart from [tag/TAG]

Resolved by updating line 58 to

'''

e.g. '[tag/TAG]…' can be used as ' ' (i.e. 0 times), 'tag/student', 'tag/TA' etc.

'''

Resolved by adding in commit suggestion

'''

List Students in Class | 'list-students /by n/CLASS_NAME'
e.g., 'list-students /by n/CS2103T Tutorial T10'

'''

Resolved by changing clear class to "clear-class"

Resolved by changing list students to "list-students", and title to "Listing all students within a class"

Resolved by retaining {:toc}

Resolved by removing redundant add

Resolved by moving ellipsis outside of square brackets

Resolved by removing address field and updating tag prefix from /t to /tag

Ok i won't be touching it in that case

Added the tags, used tag/average and tag/TA candidate to keep formatting consistent

I think it'd be better if the summary comes before the detailed explanation. As a user i wouldn't want to scroll downwards if i just want to reference the commands. What do the rest think?

I thought of that, but the issue is that the formatting will not be standardized across the table and commands. I feel that perhaps its better to caps everything so that we standardize our documentation

Issue is that the formatting will not be standardized across the table and commands. I feel that perhaps its better if we follow the actions used in the command summary?

Changed Command summary to Command list

Resolved by rewording expression to show that Tutor's Pet can return more than one user found

Will keep clear instead of changing to delete

Ok lets keep the previous

Will be keeping line 155 instead

To change to requireAllNonNull(uuid, name, phone, email, tags);

Good catch, thxs, no point editing UUID so no point adding UUID into EditStudentDescriptor.

Resolved by removing setUuid() and getUuid()

Resolved by changing line 93 to UUID updatedUuid = studentToEdit.getUuid();

To add a DEFAULT_UUID in StudentBuilder.java and convert it to a UUID for usage instead of generating a random UUID for testing

Resolved by removing UUID field

Resolved by specifically adding Uuids into TypicalStudents

RIP, forgot to remove sneaky boi

Added the following :

JsonAdaptedStudent.java

'''

    if (uuid == null) {

        throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, "uuid"));

    }

'''

JsonAdaptedStudentTest.java

'''

@Test

public void toModelType_invalidUuid_throwsIllegalValueException() {

    JsonAdaptedStudent student =

            new JsonAdaptedStudent(null, VALID_NAME, VALID_PHONE, VALID_EMAIL, VALID_TAGS);

    String expectedMessage = String.format(MISSING_FIELD_MESSAGE_FORMAT, "uuid");

    assertThrows(IllegalValueException.class, expectedMessage, student::toModelType);

}

'''

Since UUID is a built-in java class, instead of doing something like '''throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, Name.class.getSimpleName()));''' , i opted for '''throw new IllegalValueException(String.format(MISSING_FIELD_MESSAGE_FORMAT, "uuid"));'''

Possible, but the json "studentIds" in the json file will be changed to "studentUuids". Which one do you guys prefer?

I initially added a class entry, but then eventually removed them as I wanted to isolate the test cases better (Since this json file is more to test for invalidstudent rather than invalidclass.) Any thoughts?

Currently since we are using UUID.randomUUID(), the edge case that could potentially happen is a null UUID rather than that of a UUID that is not specified properly, unless the user decides to edit the uuid in the json file to a smaller uuid string. Hence i decided to only test for toModelType_nullStudentIds_throwsIllegalValueException()

Any thoughts on this?

Was thinking that a guard clause would be sufficient (Since its likely the case that a uuid is either well defined (by using Java's UUID.randomUUID(), or null))

Any thoughts on whether a try catch is needed here? Will include one if needed

Thats a good idea. I'll pull information from ALICE directly and remove the TODO comment

Resolved by keeping status quo for invalidClassTutorsPet.json (ie no changes)

Resolved by adopting suggested change

Resolved by pulling information from ALICE directly and removing TODO comment

Resolved by refactoring all occurrences of studentIds to studentUuids, including the studentIds field in model ModuleClass.java

Resolved by abstracting code inside toModelType() into two smaller methods.

Resolved by adding try catch block

Resolved by introducing checks for invalid UUIDs

Yes, it applies across all UUIDs so long they are not invalid

An example of an invalid UUID is "fffa3023-7d89-426b-ba01-" // No uuid values after dash

In contrast, a valid uuid is "fffa3023-7d89-426b-ba01-d45eaa" // Missing values after d45eaa are padded with zeros

Resolved by using existing information from ObservableList in tutorsPet

Completed

This seems to be missing the saving of UUIDs to local storage, is that planned for a separate PR?

To implement saving of UUIDs to local storage

Mostly looks good to me, just need some minor changes.

Could you also add a test case to ensure that 'EditStudentDescriptor' does not change the UUID of a student?

I think this part of EditStudentDescripterTest.java factors in that UUID isn't changed? Otherwise it will return false if the UUID changes

'''

public class EditStudentDescriptorTest {

@Test

public void equals() {

    // same values -> returns true

    EditStudentDescriptor descriptorWithSameValues = new EditStudentCommand.EditStudentDescriptor(DESC_AMY);

    assertTrue(DESC_AMY.equals(descriptorWithSameValues))

'''

Not sure if we need to get approval before we merge this in since we are trying to use a new module?

Module has been removed (Last checked 26/09/2020)

Looks great! But like what Dexter said, I am not sure whether we need to get permission first.

Module has been removed

@ypinhsuan can yours work? my one works fine

Decided not to implement UUID equality check as it may risk introducing a lot of bugs into the code

Can consider renaming function to execute_invalidPersonName... instead of Index since we are editing by name now.

Just a side note, can consider "editPatient" as command word in case we want to have an "editTask" command in future. Up to you! 😃

Good use of Optional here!

Typo @code instead of @cod

I like how you included this test!

Your implementation of editpatient supports editing multiple fields in 1 command. Can consider adding an example of this. E.g. editpatient Alex Yeoh c/Add comment d/20200303-20200315

In the example, should be editpatient not edit haha ><

Perhaps you want to consider changing the message to "Search for patients who fulfill the given criteria in Covigent." since the output may be a list of patients.

Personally, I think including an example for search via temperature range may be better since this may be more difficult for users to understand how to use.

Actually you can remove the isPersonFound attribute totally. if (personName.equals(nameToSearch) can just return new CommandResult(String.format(MESSAGE_SEARCH_PERSON_SUCCESS, personFound)) directly. If the execution of code proceeds beyond this for loop, can just throw new CommandException outside. Just a suggestion to keep your code shorter 😃

Good test coverage!

Good to include an attribute string for message usage (similar to the other commands)

Will be good to include a MESSAGE_SUCCESS attribute or something similar for the string " rooms are added in a hotel" (can refer to the other command classes to see what I mean).

E.g. in addPatientCommand your have the attribute String MESSAGE_SUCCESS = "New person added: %1$s";

So what you return is return new CommandResult(String.format(MESSAGE_SUCCESS, numPersonToAdd)) instead of the hardcoded string.

Just to clarify, this exception is thrown if the user did not call the command addnumberofrooms right?

This attribute was what I was referring to for MESSAGE_USAGE earlier. Perhaps you may need to shift this attribute to AddNumberOfRoomsCommand class instead to keep it consistent with the formatting of other command classes.

Ok actually I am kind of confused of the flow and purpose of the addNumberOfRooms command. I thought for first time user we will get them to initialise the number of rooms and that will be fixed through out the usage of the app. So addNumberOfRooms in this case should be for initialisation? It doesn't make sense if halfway throughout the usage of the app I suddenly call addNumberOfRooms command again unless I built more rooms for my facility. 😕

Actually why do you need to add code for room under AddPatientCommand?

Just curious, why did you choose to use PriorityQueue instead of something simpler like ArrayList? 😛

Good test coverage for Room!

Remember to delete this!

This will be handled by allocate person command no worries.

Nice!

If we have to convert to priority queue, do you guys think if it's better to use array list directly instead of priority queue for room list?

Right, that's true!

Should be after search patient command but it's okay for now. We will do a massive UG clean up in future as a group 😃

Will be starting with an empty "room list" or something instead of AddressBook. Just to clean up traces of address book from the code haha.

I agree with this too. There may be errors if we forget to update this variable when needed.

Neat class that adheres to format of the existing codebase!

I was just wondering why you need to have a date time created attribute for task 😛

Neat I like this!

Good test coverage.

As in I meant the attribute "DateTimeCreated" haha, like why do you need to know when the task was created 😛 . The tracking of tasks due date is by attribute "DateTimeDue".

Sounds good!

Thanks for spotting this :X

Good! We will need to add test codes for patient addition for editRoom command too.

Very neat!

Do you think it is better to write "Lists all the rooms in the application." instead? I got confused with the word 'available' thinking it meant like empty rooms without patients.

Do you want to standardise the MESSAGE_SUCCESS with that of list patient command? Like "Listed all rooms."

To add on, can even add a "Please initialise the number of rooms using initRooms command".

Allocates*, edits* and existing*. Can change only when we do our massive UG clean up also.

Removes*. Comprehensive examples given here!

Do you think we should put this function under EditRoomCommand class or should it be the responsibility of RoomList class?

Nice check! Thank you for adding this 😃

I am not sure if it is a good idea to make this function static and expose it to the outer world, considering how you also need to call the getRoomList() function from the model to get the ObservableList as your parameter for this method so the roomList will be from your existing RoomList class anyway 😕. Just a thought to consider!

Very comprehensive test cases in this class. Well done!

Just curious why we need this toString() method. Is it for testing haha? But can keep it anyway.

Hmm I was thinking of having the function "isPatientAssignedARoom" in the Model interface instead. Since the Model contains roomList and the patientRecords, Model knowing whether isPatientAssignedARoom kind of makes sense (essentially it will call on another isPatientAssignedARoom method in the roomList class to get the result).

I think the key here is that the command should only know of Model and should not be accessing roomList directly. (like the Law of Demeter that we learn for this week) That's what I intend to improve for my deletePatientCommand because I also accessed the patient list directly from model, but I think it's not very well designed since there's a deletePatient command directly from model (original code from address book).

Yep no worries!

Typo: "tasks with"

Can consider "Searches all tasks before the given date."

Searches task(s)* -> I think the output may be more than 1 task right?

MESSAGE_SEARCH_TASK_SUCCESS

I think you will have to display the output in the "Tasks" tab instead. So you shouldn't be returning the result in the bot's response.

Good!

Can consider adding some success test cases and check if the output tasks are as intended.

Is it better to use the prefix_room_number from RoomCliSyntax instead since room number is an attribute of room?

Seems like success cases are covered in equals() method test.

Sounds good. I've changed the names to AddPatient instead of AddPerson for better clarity. However, if we are going to implement Staff, maybe can extend from Human instead of Person because there will be too many changes required to replace Person to Patient.

There's a possibility that comment may be a null object (since it's an optional field) and .trim() method exists for String class so it wouldn't work if comment is a null object 😛

That's a very good suggestion, will use this function in future! 👍

I followed their existing format. 😛 Since checkstyle doesn't give error, I assume it's fine?

Yes!

Yep I will change to add patient instead!

Thanks for pointing out!

I think the temp should write but the age probably no need haha.

Yes, good suggestion. We definitely should adhere to this in the next UG update!

Yep, should take note of the standardisation in UG.

I agree with Ming De I think we should stick to making sure patient does not have the same name. If we delete by index, in the case of 1000 entries, the user may have to scroll too much to find the patient (using searchpatient command doesn't show the index also 😕).

If we just ensure patient cannot have the same name, then all we need to change is the isSamePatient(...) function (to guard against duplicate patient being entered). If we need to check for the age and phone number identifiers, then we may need massive changes for deletepatient and editpatient command 😕.

Well, we can justify that the odds of patients having the same name is really very slim haha (https://www.quora.com/What-is-the-probability-of-two-people-having-the-same-name-and-also-living-in-the-same-city)

Yep yep the search room command is incomplete as I've mentioned haha. The main thing in this PR is just refactoring.

I think we will have to shift checkIfRoomPresent() to Model class also. But I will fix that in my next PR for SearchRoomCommand. The key for this PR is just refactoring.

I will change all the empty line thing. Thanks for pointing out!

Sure, I will change to "All patients are listed."

I realised search room for now is just search by room number. So it only returns 1 room haha.

I realised I should be making the pull request from my fork instead of editing directly in this repo for UG.

Incorporated with editRoom command.

'''suggestion

Tertiary student

'''

'''suggestion

'''

Don't commit .iml files.

'''suggestion

Tools:

'''

Newline at EOF.

parseEmail should be renamed to parseDate with the corresponding parameter renamed. Same for all other instances.

parseTag should be renamed to parseCategory with the corresponding parameter renamed. Same for all other instances.

parsePhone should be renamed to parseAmount with the corresponding parameter renamed. Same for all other instances.

'''suggestion

 * Parses an {@code String amount} into a {@code Amount}.

'''

'''suggestion

 * @throws ParseException if the given {@code amount} is invalid.

'''

'''suggestion

 * @throws ParseException if the given {@code date} is invalid.

'''

'''suggestion

 * Parses a {@code String category} into a {@code Category}.

'''

'''suggestion

 * @throws ParseException if the given {@code category} is invalid.

'''

'''suggestion

 * Parses {@code Collection<string cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> categories} into a {@code Set<category cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf">}.

'''

parseTags should be renamed to parseCategories with the corresponding parameter renamed. Same for all other instances.

'''suggestion

The UI consists of a 'MainWindow' that is made up of parts e.g.'CommandBox', 'ResultDisplay', 'TransactionListPanel', 'StatusBarFooter' etc. All these, including the 'MainWindow', inherit from the abstract 'UiPart' class.

'''

'''suggestion

:information_source: **Note:** An alternative (arguably, a more OOP) model is given below. It has a 'Category' list in the 'AddressBook', which 'Transaction' references. This allows 'AddressBook' to only require one 'Category' object per unique 'Category', instead of each 'Transaction' needing their own 'Category' object.

'''

'''suggestion

ecp -> ec ++: index, editPersonDescriptor

'''

Let's update the diagrams at a later stage.

'''suggestion

Simply add the following to 'seedu.address.ui.PersonCard'.

'''

Let's revert this change since we will most likely be deleting tutorials eventually.

'''suggestion

   Person editedPerson = createEditedPerson(personToEdit, editPersonDescriptor);

'''

Could you import EditCommand.EditTransactionDescriptor instead?

'''suggestion

Adds an income to the finance tracker.

'''

The command result display is not big enough to display everything, nor can it be scrolled. Instead, the words are just cut off.

Should something else be used instead of menu? Having to click the Menu, then the MenuItem for tabs is very clunky. An alternative is JavaFX TabPane.

I don't think the help menu should be grouped with the other options as the other options are tab headers while the help menu is a menu.

This extra line break can be removed.

'''suggestion

 * Opens the analytics window.

'''

Missing full stop.

panelLabel does not seem to correctly reflect the chosen tab.

Note how Income is selected, yet the label displays Expense.

If the JavaFX TabPane were used, I don't think you need to write such hacky code.

'''suggestion

 * Creates an {@code ExpensePanel} with the given {@code ObservableList}.

'''

Should this class be dealing with Expense objects instead of Transaction objects?

'''suggestion

 * Creates an {@code IncomePanel} with the given {@code ObservableList}.

'''

Should this class be dealing with Income objects instead of Transaction objects?

Any reason why these imports aren't grouped with the others?

Move this import up with the other imports, and leave a line after the last import.

This might be due to the bug found by @zhaojj2209 whereby switching tabs works only for the first time since the application was started.

Unable to reliably replicate this. I'll close this comment for now and keep and eye for this bug occurring in the future.

@siddarth2824 will be addressing this in a separate PR.

See https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/75#discussion_r501141034.

This will be changed in a future PR once the Expense model is more fleshed out.

This will be changed in a future PR once the Income model is more fleshed out.

Add full stops? Same for the other comments.

Possible to add backticks to parts which reference code? i.e. AddCommand

'''suggestion

        + ": Deletes the expense identified by the index number used in the displayed expense list.\n"

'''

'''suggestion

        + ": Deletes the income identified by the index number used in the displayed income list.\n"

'''

'''suggestion

Deletes an income identified using its displayed index from the finance tracker.

'''

'''suggestion

Deletes an expense identified using its displayed index from the finance tracker.

'''

Currently, these specific success messages are being shown. Should these be removed?

This value depends on the number of entries in our test data though. How do you propose we get around this without hard-coding?

'''suggestion

public void execute_showsAllExpenses() {

'''

Sounds weird otherwise. 😕

'''suggestion

public void execute_showAllIncomes() {

'''

Is this line break intended?

pitest can be added at a later time when we get around to achieving full coverage.

My bad, removed test.

I thought that the GITHUB_TOKEN secret was being blocked due to this PR being from a fork. Seems to also happen with branches on this repo. 😕

Apparently, GITHUB_TOKEN cannot be used; must be a GitHub user's personal access token.

Setting the baseurl to be the repo name is necessary for GitHub Pages to function.

CI job is able to run now. Tested on a branch of this repo as PRs from forks are unable to access secrets.

See https://github.com/AY2021S1-CS2103T-W16-3/tp/runs/1160121842.

@yongping827 The corresponding comment in AddIncomeCommandParser does not have a full stop, should I still add it?

This was left as such to mirror the corresponding change in #56. Can I confirm this is not intended?

Edit: Never mind, just saw https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/63#discussion_r499263026.

Full stops to be added separately. This is honestly very low priority.

Hmm, I went with ListTransactionCommand as the commands shouldn't have to care about the state of the UI; that's the job of the FinanceTrackerParser.

I was wondering why IntelliJ was throwing warnings for the pitest task in build.gradle. See https://github.com/szpak/gradle-pitest-plugin#eliminate-warning-in-idea. For documentation purposes only; I agree that this change feels less readable.

Requested for permission to use the external library at nus-cs2103-AY2021S1/forum#253.

LGTM!

While the current deployment process works with personal access tokens, @wltan brought up the point that we should have it work with the repository token GITHUB_TOKEN instead. This is a more ideal solution, especially if Prof Damith were to adopt the use of jekyll-spaceship.

Relative links are currently not working. Trying to find a fix.

PR has been picked up by the grading script.

Closed with #50.

Screenshots

Desktop:

Mobile:

Could you rebase to address merge conflicts before we start reviewing?

UPPER_CASE shoudnt be in markdown

There is no line breaking here.

LGTM

quick start should start with ##

The contents should be hyperlink

I will add the hyperlinks

the list should have the ``

I think there should not be indentation here

Should it be "the list of all tasks"?

should it be "list all tasks"?

Same as above

are we going to skip displaying or just display an empty list? If we are just displaying an empty list, is it still considered an extension?

list all tasks

the list of all tasks

same as above. i'll ignore it for the following parts

Is 'exit' inside the use cases? should we add it?

This is still not changed

The class here is still not changed to Task

Why extra indent here?

Why extra indent?

same here

nice!

Should we use trimmedTitle instead of trimmedName?

Do you wanna change "address book" to "PlaNus" now or do it later?

Does the name "fullTitle" make sense?

Why extra indent?

just to check, the corresponding UI class variable is also changed right?

What's the rationale of changing it to Amy?

agree. and better put date as yyyy-mm-dd

the attribute in the userguide says date: tho

Should have clearer documentation here

Clearer documentation pls.

Why is this private variable in between to public variables?

I think a better documentation is : construct an empty Description when user didn't provide the description field. Caveat: Only called when the user didn't key in this field.

Same. private variable in between public variables.

Same. I think the documentation can be better.

I would recommend making this documentation clearer as well. Add another line to specify that this is only used to construct an empty Phone when the field is not in the task.

Nice that you changed the UML as well.

oops, I think this should have been "description". My bad I forgot to change this. Can you change it?

Should we really allow type to be any string? Or just one word letters connected by '-'

Is there a reason to keep a counter? Don't we just need to traverse through all the types?

iirc there's a "requireNonNull" method there? Can we use it?

hmmm maybe you can consider using the join method?

Maybe you can consider using the "join" method?

I think we are changing all these to PlaNus

Should I just delete it or mark it as coming soon?

got it

nice catch

Just mark it as done when you finish your part

What are left to be done for the use cases? Maybe can add inside description?

Completed

Looks like theres a formatting issue here

I think we should turn the 4 into a constant, i.e. private static final CALORIC_MULTIPLIER = 4. Same for the rest of the macronutrient classes

Similar to the toString function, I think we can factor this out to the superclass. If we do that then we can also change all its protected access modifiers to the ideologically superior private.

Looks like you're using the assertion incorrectly, it should be

assert amount >= 0 : "amount cannot be negative"; instead.

Currently it will print the error message regardless of whether the assertion passed or failed.

Also a side note: we should use the java logger instead of System.out.println apparently.

Maybe you could add a TODO comment on what kinds of methods to add to this class/how it should be used.

e.g. // TODO: Food FoodMethodHandler.combineFood(Food firstItem, Food secondItem) or something

IIRC we need to write javadocs for each class according to the module specifications.

IMO we should keep this as the string "Carbohydrate" instead. Makes the code more explicit.

Same for the other macronutrients

I think we should @override the object.equals method instead. Having 2 methods claiming to do the same thing will invite bugs.

This method only makes sense if we have unique objects in the list, maybe you could change the target to the Index of the object in the list, or require all objects in the fridge to be unique.

Same as the setFood method

I think we should rename this to macronutrientType or something so we don't confuse this type with a java type.

Assertion here could be more descriptive (e.g. "Macronutrient type cannot be blank"), since we are changing name to type.

We should be checking for referential equality here i think? i.e. assertTrue(expectedFridge == fridge), not sure if object equality is intended.

Same as below comment

typo in caloriMultiplier.

Also I think you MacronutrientStub or something would be a better class name. The current one sounds like this is an object that creates other macronutrients.

Should remove this or put a TODO comment if its blank

I think this should be withFood or something

Also is using person here intended?

I think a comment would be good here to say it takes the name from the class name

Maybe we should change int index to Index index, i.e. the class from AB3.

I think we should keep this as just assertThrows instead. We should stick to the static asserts for tests

same for all the other test files that were changed

hmm we may be able to abstract this part a bit more in the future, just a note, no need to change now

this is not an email !

I think we should be using getNutrientValue here instead?

this method violates DRY, we should really remove this

this is important

can't see clearly in the diff but I think we're missing a javadoc here

We should extend Exception instead because they are checked

extending RuntimeException will lead to a lot of very painful uncaught errors

Same as the other DuplicateFoodException, RuntimeException is very spooky

I guess this is valid but should we leave this or change it to mcspicy or something

same as invalidFoodMcGymmy, i think we should use actual food names here

same as the .json files, we should change to food values

Maybe change the names and tags here as well to food

As above, I think we should change these names

could abstract this into a method hasDuplicates in ReadOnlyMcGymmy or its superclass

this should be if (food == comp) for referential equality (the is operator in python).

Comparing the hashcodes does not guarantee the food and comp are the same object in memory.

As with the other files, should change this to food.

should change all these to resemble the typical values, except with an underscore, space etc.

i.e. each test should only have 1 reason to fail.

Should use the module convention, i.e. protein_isValid_correct or something

Same for the methods in the other test classes

Need to change these examples too

We should change all these names/values to food ones.

Should be Carbs.MESSAGE_CONSTRAINTS instead here, same for fats below

i think toString might override the java object's default method, so that might cause problems (including confusion in developers)

maybe we could use the module convention toString_isCorrect or something instead

Hmm looks like I may have accidentally removed this during the safe delete or something, will update and restore it.

Yes that seems to be better. Good catch.

I'll add a couple more tests for this, along with some unit/integration tests with the Commands.

Nope, if you try to declare multiple of the same parameters there will be an assertion error somewhere.

Technically you can work around this, by declaring a new class that contains the two parameters, e.g.

the command:

'takeTwoIntegers 1 2'

may have a parameter

'private Parameter param = this.addParameter(..., IntPair::parse)'

and the IntPair class is as follows:

'''java

class IntPair {

public int a;

public int b;

public parse(String str) {

IntPair result = new IntPair();

result.a = ...

}

'''

Don't think any of our commands will involve these kinds of parameters, but if they do we may have to either redesign the command or the parser. (We'll solve that problem when it happens)

Yes, for the first one the 1 will be considered as input to the -n parameter, and the unnamed parameter will have no input (so there will be an error).

I'll add this and the comment below into the PR's docs.

No point imo since I'm not reusing the string outside this function.

Also would add more noise which we're gonna change anyway (when we change people to food).

good job

Added a skeleton to Duong's PR. Should be able to add on from there while avoiding merge conflicts.

(related note: can someone approve that PR? don't think I can do it because I also contributed a commit.)

We should use the same format as the one from AB3 (copied over below).

Use case: Delete food

MSS

User requests to list persons
AddressBook shows a list of persons
User requests to delete a specific person in the list
AddressBook deletes the person

Use case ends.

Extensions

2a. The list is empty.

Use case ends.
3a. The given index is invalid.
- 3a1. AddressBook shows an error message.
  
  Use case resumes at step 2.

The CI test says that you are missing the shadow plugins

Seems like a github actions issue. This happens from time to time.

Closing and reopening the PR should restart the CI process (hopefully fixing this).

LGTM

I think I can do this easily using the new logic architecture

Preliminary findings:

This is actually a symptom of a much bigger problem: the code is violating SRP and Seperation of Concerns.

Context

Right now the model classes (i.e. 'Fridge', 'UniqueFoodList') are throwing these 'RuntimeExceptions' when the food being added is a duplicate etc.

The reason why this has not been a problem so far is because (for the case of the 'DuplicateFoodException') the 2 classes that add to the food list/fridge, the 'AddCommand' and 'EditCommand' have guard clauses that throw an error when a duplicate food item is being added.

'''java

if (!(foodToEdit.equals(editedFood)) && model.hasFood(editedFood)) {

throw new CommandException(MESSAGE_DUPLICATE_FOOD);

}

'''

this one

Frankly this shouldn't be the job of the 'EditCommand' or 'AddCommand' and now we have to include extra tests to ensure these guard clauses work.

Using the current design, in the future if we add more 'Commands' or other functionality that add 'food' items to the 'model', we will have to keep in mind to catch these duplicates ourselves else oopsies! the program will explode because 'Fridge' threw a 'RuntimeException'.

It is my firm belief that since Java so benevolently offers to handle these problems for us (with checked exceptions), we forgetful humans should gracefully accept its help.

Note: I just remembered we're allowing duplicates now but the above argument still applies to the other 'FoodNotFoundException'.

Proposed Change

Recall the current guard clause used:

'''java

if (!(foodToEdit.equals(editedFood)) && model.hasFood(editedFood)) {

throw new CommandException(MESSAGE_DUPLICATE_FOOD);

}

'''

If 'DuplicateFoodException' is thrown as a checked exception, we can easily wrap it in a try-catch:

'''java

try {

// do the adding here or wtv nonsense

} catch (DuplicateFoodException e) {

throw new CommandException(MESSAGE_DUPLICATE_FOOD);

}

'''

Copied over the justification from AB3

'''

PersonNotFoundException is a checked exception. It is thrown by methods

such as 'UniquePersonList#remove(Person)',

'UniquePersonList#setPerson(Person, Person)' etc. to signify that

certain preconditions were not met -- namely that the person provided to

those methods does not exist in the address book/UniquePersonList.

Using checked exceptions for such a purpose is slightly useful in that

it will force callers to handle the case where the above preconditions

are not met, such as when the methods are called with invalid user

input.

However, it imposes a HUGE cost on callers where the preconditions are

already known to be met (e.g. in test code, or when the user input has

already been validated before hand). In such a case, callers are forced

to add try-catch blocks around the method call even if they know that

the exception will never be thrown, bloating up the code. It is also

impossible to test the catch blocks as well since correct code will

ensure that the precondition holds and thus the exception will never be

thrown, leading to reduced code coverage.

Checked exceptions also don't work very well with the Java Streams API,

since the API doesn't accept lambdas which could throw checked

exceptions.

In AB-4, the amount of code which benefits from PersonNotFoundException

being a checked exception is much smaller than the amount of code which

is negatively impacted.

As such, let's make the tradeoff in the other direction, by making

PersonNotFoundException a RuntimeException. New callers could forget

to check that the preconditions hold before calling the methods in

question (although test cases should catch that), but this is balanced

out by the huge benefit of having more concise and testable code.

'''

Only argument I agree with is the 'Java Streams' one, but we can work around this.

We can discuss this in the next team meeting.

Shouldn't the text name for this be matriculationNumber?

Maybe INVALID_MATRIC_NO could be changed to INVALID_MATRIC_NUM instead?

Shouldn't there be a method call withMatriculationNumber(...) as well?

Perhaps MATRIC_NO_* could be changed to MATRIC_NUM_*?

Should there be an awkward line break right after MATRIC_NO_BOB? Same issue with the other test cases.

Shouldn't there be a method call withMatriculationNumber(...)?

Shouldn't the message be ATAS has deleted all students! or something like that?

Perhaps these could be renamed to ACCEPT_COMMAND_FULL, ACCEPT_COMMAND_SHORT, REJECT_COMMAND_FULL and REJECT_COMMAND_SHORT?

Perhaps comments could be added to explain what you're testing for each statement (+ expected result)?

I noticed a similar issue previously. Maybe more comments explaining each test case could be added?

Do you think it would make more sense to include the use of the abstracted wrapper class Index here?

Would this make more sense to put in an if-else block (with comments to describe the else block)?

Should this unncessary extra line break be removed?

Maybe these could be renamed to PREFIX_SESSION_NAME and PREFIX_SESSION_DATE?

Shouldn't the form be dd/MM/yyyy?

Maybe some comments could be added here to more clearly explain the thought process?

Unnecessary additional line break?

Maybe these two if blocks could be merged into one?

Would it make more sense if all instances of SESSIONNAME got changed to SESSION_NAME? (same for SESSIONDATE and SESSION_DATE?

Unnecssary whitespaces?

Unnecessary whitespaces?

Unnecessary extra line break?

Unnecessary line break?

Unnecessary whitespaces?

Shouldn't this be renamed to Editing a student to fit the rest of the item naming?

Maybe this class could be made public?

Shouldn't the command participate be in all small letters?

Shouldn't this be enterses?

Shouldn't this be presence?

Shouldn't the command here be editses?

Maybe you could add a TODO for next time?

Thanks for pointing this out, as well as in other places!

Should this be project book?

Should we replace getTempFilePath("ab") to getTempFilePath("pb")?

Should the variable be named pb instead of ab?

Should this be named pb instead of ab?

Should the indentation here and in the method below be 4 spaces from the parent comment?

Is there a reason for adding this? These lines were removed in the previous PR as we removed those fields

Just to confirm, the only time this test will fail is when the description is empty right

Should there be an empty line before the @param tag?

Agreed, maybe we can create a new PR for this after merging all the important features for milestone 1.1.

Should there be an empty line before the @params tag?

Is the requireAllNotNull() method here supposed to check the description as well?

I believe the description parameter is not optional as of now, right?

I think the code here should be rearranged so that it is a little clearer. The modelName variable should be delared after the if (!Name.isValidName(name)) block

Minor typo here

Is there a variation of this test that you have written?

Not sure if this Javadoc adheres to the coding standard

Javadoc here should be StopCommand instead of StartCommand

Should these two checks be extracted out into another method to keep the equals() method here clean?

Should the javadoc here be more descriptive?

Should have an empty line before the @param tag

Should the equals() method below check whether both timers are equal?

By below I mean in the Project.java class, can't really highlight it from here.

Javadoc comment needs to be updated to include timer

Why not just use the otherProject variable here?

There should be an extra line before @params tag and a description for what is returned after the @return tag

Should include an empty line before the @params tag

Can I just check what the reason for changing this was?

I think the commands here can be highlighted once again. For example

/home brings the user to home dashboard

Should we replace these images with @khoodehui 's mockups? At least the ones that are ready

If I'm not mistaken, all existing tags will be removed and only the tag normal will be left. Should we mention this?

Should we separate format and examples into two columns?

Can replace this with @khoodehui 's mockup as well

Should these sentences end with a fullstop

Should there be a big space here?

This is something minor but I feel that it might be simpler to just do

Objects.hash(model, weeklyTotalTimePerProjectStatistic, Arrays.hashCode(statistics)).

Small issue but can you just change ProjectBook to addressbook here because the issue comes from there

I agree with Farrell. I think it might be better to keep it in deadline.

Do you think this method will be used elsewhere in the future? If it is, it might be a good idea to move it over to StringUtil.java

I think the javadoc for this method should be updated to reflect the new name

As discussed, description will be implemented as an optional parameter instead. A new PR will be created for this issue.

would it be better to remove tutorial slots part cause we don't really have a tutorial?

'''suggestion

'''

'''suggestion

'''

'''suggestion

        + PREFIX_TAG + "CS2103 Tutorial "

        + PREFIX_TAG + "Experienced";

'''

'''suggestion

Edit Student | 'edit INDEX [n/NAME] [p/PHONE_NUMBER] [e/EMAIL] [t/TAG]…'
e.g.,'edit 2 n/James Lee e/jameslee@example.com'

'''

I think someone forgot to change this previously...

'''suggestion

'edit-student 1 t/@johndoe e/johndoe@example.com' Edits the telegram handle and email address of the 1st student to be '@johndoe' and 'johndoe@example.com' respectively.

'''

Hmm... I think putting it at the top would be more convenient for users to just take a quick look... But the word 'summary' makes it feel like we concluding something so its weird for it to be at the top... Why not change the heading to like 'Command List' or something equivalent?

I think keeping the previous would be better also... The previous formatting was based on the original user guide we got for the address book... so i think it is fine that the table and commands not consistent.

I think clear is used to clear all students though... delete is meant to delete a specific student...

'''suggestion

Listing all students : 'list'

'''

'''suggestion

Clearing all students : 'clear-student'

'''

'''suggestion

Editing a student : 'edit-student'

'''

'''suggestion

Clearing all classes : 'clear-class'

'''

Perhaps it might be good to follow the same format as 'DeleteStudentCommandTest' to be consistent?

'''suggestion

Contains integration tests (interaction with the Model, UndoCommand and RedoCommand) and unit tests for
{@code EditStudentCommand}.

'''

I like how you specify that clear removes all students!

'''suggestion

public void addStudent(Student student) {

    students.add(student);

}

'''

Would this be a better name?

'''suggestion

public void resetData_withDuplicateModuleClasses_throwsDuplicateModuleClassException() {

    // Two moduleClasses with the same identity fields

    ModuleClass editedCS2103T = new ModuleClassBuilder(CS2103T_TUTORIAL).withStudentIds().build();

    List<student cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> defaultStudents = TypicalStudent.getTypicalStudents();

    List<moduleclass cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> newModuleClasses = Arrays.asList(CS2103T_TUTORIAL, editedCS2103T);

    TutorsPetStub newData = new TutorsPetStub(defaultStudents, newModuleClasses);



    assertThrows(DuplicateModuleClassException.class, () -> tutorsPet.resetData(newData));

}

'''

Would this be a better name?

'''suggestion

//// Student related tests

'''

'''suggestion

/**

Wraps all data at the application level.
Duplicates are not allowed (by .isSameStudent and .isSameModuleClass comparison).

Perhaps a line break here will make it look less cramp?

'''suggestion

    Index outOfBoundIndex = INDEX_SECOND_ITEM;

    

    // ensures that outOfBoundIndex is still in bounds of Tutor's Pet list

    assertTrue(outOfBoundIndex.getZeroBased() < model.getTutorsPet().getModuleClassList().size());

'''

should this be

'''suggestion

// identity fields

'''

same for the data fields comment

Perhaps this comments and the few below should start with lower caps?

'''suggestion

    // one keyword

'''

would it be better to leave a line here?

'''suggestion

     // manually link first class to first student

    ModuleClass moduleClass = model.getFilteredModuleClassList().get(0);

    Student student = model.getFilteredStudentList().get(0);

    Set<uuid cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> studentUuids = new HashSet<>(moduleClass.getStudentUuids());

    studentUuids.add(student.getUuid());

    ModuleClass modifiedModuleClass = new ModuleClass(moduleClass.getName(), studentUuids);

'''

perhaps this comment should start with lowercase?

''' suggestion

private TypicalModuleClass() {} // prevents instantiation

'''

perhaps this should start with lowercase?

'''suggestion

// manually added

'''

Perhpas this is missing a full-stop?

'''suggestion

// Manually added - Student's details found in {@code CommandTestUtil}.

'''

perhaps this should start with a lowercase?

'''suggestion

public static final String KEYWORD_MATCHING_MEIER = "Meier"; // a keyword that matches MEIER

'''

Perhaps this would be a better naming?

''' suggestion

public void execute_indexSpecifiedListIsNotFiltered_showsStudentsInClass() {

'''suggestion

public void execute_invalidIndexSpecifiedClassListIsNotFiltered_throwsCommandException() {

'''

Would it be good to specify which list?

'''suggestion

/**

 * Removes all {@code studentUuids} from each and every {@code ModuleClass} in the class list.

 */

'''

Perhaps student manager would be a better name?

'''suggestion

/**

 * Removes all {@code Student}s from the student manager.

 * Also removes all {@code UUID}s from each {@code ModuleClass}.

 */

'''

I am not sure about this... But should the full-stop be after the example?

'''suggestion

/**

 * Ensures that Tutor's Pet will not be able to boot up given an incomplete {@code UUID}

 * (e.g "0c527a3f-8a6f-4c16-b57d-").

 */

'''

Perhaps it will be a good idea to keep the formatting consistent with the subsequent tests?

'''suggestion

/**

 * Ensures that Tutor's Pet will not be able to boot up if two or more {@code Student}s are found with the same {@code UUID}.

 */

"something is wrong" feels a little generic.. Perhaphs it is good to specify?

'''suggestion

        // Check that the set of student UUIDs within a class is a subset of that of

        // all student UUIDs in uniqueStudentUuids. Otherwise, Tutor's Pet would not

        // boot up due to data corruption.

'''

would canUndo_noPrevState_returnFalse() be a better name for this test so as to keep it consistent with the other namings?

would undo_noPrevState_throwsUndoStateException() be a better name for this test so as to keep it consistent with the other namings?

I guess the refactor cant detect that... THANKS!

HAHA cause dexter added that in his i think

That test was missing so i just added it and made it consistent with what was in EditStudentCommandTest

Perhaps it would be good if we insert our website here.

Perhaps we could keep keep this so we can change it individually

Perhaps we could keep keep this so we can change it in future

Perhaps we should keep the product name consistent, to Insurance4Insurance, I4I

Perhaps we can add a "Coming soon." here

Perhaps this could be "Adds a client to I4I."

Perhaps this could be Format: add n/NAME p/PHONE_NUMBER e/EMAIL a/ADDRESS [o/NOTE] [t/TAG]…

Perhaps this could be

Examples:

add n/John Doe p/98765432 e/johnd@example.com a/John street, block 123, #01-01
add n/Betsy Crowe t/friend e/betsycrowe@example.com a/Newgate Prison p/1234567 t/criminal o/This client is new.

Perhaps this could be Lists the entire list of clients in I4I.

For now, for v1.2, I think we would try to minimise the amount of fields to delete, so we would add in NOTE and keep the rest. However, this may change in v1.3/1.4, so we can update the UG then.

We can add back the "Tip" for tag if you are okay with the above change.

Perhaps we can format INDEX to INDEX.

Perhaps we can edit to Example: list followed by delete 2 deletes the 2nd person in I4I.

Perhaps we can add a Coming soon here

If you are okay with above edits to add, perhaps we can change to this:

Add | add n/NAME p/PHONE_NUMBER e/EMAIL a/ADDRESS [o/NOTE] [t/TAG]…
e.g., add n/Betsy Crowe e/betsycrowe@example.com a/Newgate Prison p/1234567 o/This client is new. t/friend t/criminal

Perhaps we can add in help as well, since we are implementing in v1.2

Help | help

Perhaps we can add this back, since we are implementing help in v1.2

But we would need to update the image to https://ay2021s1-cs2103-t16-2.github.io/tp/UserGuide.html

Perhaps we should change this to

Wraps all data at the client-list level

Would it be better if we have multiple tags and 1 note? i.e. the ... comes after tag instead

Perhaps this can be updatedNote?

Perhaps the message could be "Notes can take any values, and it should not be blank"

Perhaps this can be package seedu.address.model.person;

Perhaps seedu.address.model.note.Note can be simplified to Note

Perhaps we can return noteName

Perhaps this could be " "?

Perhaps we don't need to add an invalid tag here

Perhaps we could add back the space here String ...

Perhaps we should add back the space here?

Perhaps this should be 8 spaces relative to line 22? But not sure why the original AB3 passed checkstyle

Similar concept for the rest of the lines in this file

Alright got it

Reopening this issue as PR was accidentally merged

Note that this PR was merged by accident, reopening issue and creating another (similar) PR

Resolves #6

Ensure that source should be <= 50 characters.

Update URL and image in UG

address
email
phone

Agreed. The user might not know the exact start and end dates of his semester, and we also do not need to take into account the user entering invalid start and end dates if we provide them for the user instead.

AY20/21 Sem missing a '1'

*Duration is measured in hours and must not exceed 24 hours

Same rephrasing as above comment

*has

Can omit the second line if we are providing them with the start and end dates?

Will listpresettasks be more intuitive since it will be a list of preset tasks?

Remove a Module and all its Module Tasks from the Module Schedule

*load all its Module Tasks

If its "CS2103T", maybe we can change it to MODULECODE

Do we have a method to list all the task templates or will the GUI take care of that?

Use either remove or delete

Agreed

*listmymodules

List all the modules that the user has selected.

tasks*

Maybe can change to List all tasks instead of Listing a Task Schedule? As schedule itself refers to a list of tasks

Think you might have added an extra '.' at the end!

Should it be 'parameters' instead of 'parameter'?

Should SERVICE_CODE be added here?

maybe can replace 'additional' with another word, or just write 'displays a success message'?

*updates

*requests

*deletes

*match

*shows a, *match

*an

*displays

*an

*At least one service exists in the service list

Does it guarantee an empty service list as well?

maybe can change description to details?

*service

*GrAB3 shows the list of expenses sorted in ascending order

I'm currently using the Tag Class provided by AB3 which does not handle white space, will handle it subsequently if it is consistent with the other features using the same Tag Class.

Noted, I have changed it from a character to a String. Thanks for pointing out!

Can you change this to docs/UserGuide.md

Should we remove this *{More to be added}* line?

Can I ask what is gradeDist? Is it grade distribution?

Can I check why we are checking if other is instanceof Name? We should only check if other is also a Location right

Another instance here. I think should be other instanceof Location right?

Should we use Address Class for Task as well? I am unclear if there is a difference between Location and Address class class haha

Just curious, why did you remove the (String) typecasting?

Need to fill in return value here

Can I check, what is the difference between setModule and setModules? They are for different use cases?

Should ass throws ModuleNotFoundException, DuplicateModuleException to the method declaration right? Same for several other methods.

Just thinking. Shoudl we change this TrackItManager? Or TrackIt? haha. If it's too troublesome to change then nevermind.

Ok then we just leave it first. If need be we will let the others add in these methods when they implement further

Maybe we should add throws NullPointerException in the method declaration? Or remove that line in comments.

Can delete the @return here as well, or add the return value

Is this method supposed to be empty?

Can I check why this is removed? Are we leaving it to later to implement?

Why is @Override removed? The getTaskList method is from ReadOnlyTrackIter so we should keep the @Override right?

Is this the final PR for concrete classes? If yes we should re-enable checkStyle

Can remove this comment

Same. Can remove this comment

Here too. Remove comment

Remove comment

Should MESSAGE_USAGE be L list: sample? Maybe change to another word like CS2100 to make it clearer?

Remove Comment. Maybe change MESSAGE_USAGE to have a clearer keyword instead of sample

Should be There is no module matched to the given module code. Missing the to.

Maybe can add an example, like in AddContactCommand

Same here. Maybe should add example.

Same here. Maybe add example

Just curious. Is there a need for PREFIX_TYPE when there is already a PREFIX_NAME? OR this is just for clarity when reading code?

Since the function is parseTime, maybe should change Lesson.DATE_MESSAGE_CONSTRAINTS to Lesson.TIME_MESSAGE_CONSTRAINTS? Just a suggestion haha

Yes this sounds better

Ok I tried to change to this but err java gives me an error: Illegal escape character in String literal

What do you mean by view it by Id. Task doesn't have ad id associated with it haha

Hmm actually I think writing note/ will be very cumbersome, and may confused users because they are used all 1 letter not 1 word

Hmm ok then can I suggest we stick to r/ for remark, I believe note/ is too cumbersome

Hmm I am following the standard comment format. I think better to keep it this way for now

Ok. I suggest for now we leave it as is. Since Lesson and Task have a date component, we can standardise once both have been merged in and refactor it to be one of the 'commons'. Then that will be cleaner. Maybe leave for v1.3

I think that will require some refactoring. I suggest I create an issue for it and we settle in v1.3

Yes but apart from checking not null, we must also check if the fields fit the specific requirements, which differs for each requirement. Maybe we can look into it, but probably for v1.3

There is no description class. Description is stored as a string thats why I used String

There is no Weightage class. Weightage is stored as a double thats why I used Double class.

Yes ok I will change it

Just finished refactoring. I think we can merge this first then I can create another PR where I rename the files to AddContactCommand? and etc

Also rmb to fix issue brought up by Weihong. criminal lawyer is not alphanumeric.

https://discordapp.com/channels/@me/763052081266950177/763128915186155520

Actually does the user need to list all modules first before adding a new one ah?

Maybe an empty list shouldn't affect the addition of a new module? If not a user can never add his first mod since the list will be empty then LOL

'''suggestion

Duplicates are not allowed (by .isSameModule comparison)

'''

Should we update the default pathname to gradpad.json or something along those lines?

I think we can update the javadocs for these changed methods 😁

'''suggestion

 * Adds a new {@code Module} to the {@code GradPad} that we are building.

'''

This test fails cos 2 Modules shouldn't be equal if they have different module codes. Perhaps we can remove the module code change

'''suggestion

    Module editedAlice = new ModuleBuilder(ALICE).withTags(VALID_TAG_CORE)

'''

I think we need to update TypicalModules.ALICE cos the tests can't build since ALICE is an invalid module. It'll probably be good to rename it too LOL cos alice is still a person's name

Maybe we can rename the dummy path since we're no longer building addressbook 😁 Same goes for the change on line 46 below

'''suggestion

    userPrefs.setGradPadFilePath(Paths.get("gradpad/file/path"));

'''

Same as prev comment

Maybe we can remove the regex split? It doesn't do anything since module codes never have spaces in them. Maybe we can just create an array with the module code in it as a single string:

'''suggestion

    String[] keywords = { CS2103T.getModuleCode().moduleCode };

'''

This is exactly the same as the 'CS2103T' in the prev line HAHAHA so it'll always return true LOL

In this test, we are seeing if modules with the same module code and MCs, but different tags, will be considered the "same" by GradPad, so maybe we can just change the tag:

'''suggestion

    Module editedModule = new ModuleBuilder(CS2103T).withTags(VALID_TAG_NON_CORE)

'''

Nit: I think we need to update the javadocs for this method

Let's change these example tags HAHA altho I wish modules could give me friends and money...

'''suggestion

public static final String MESSAGE_DUPLICATE_MODULE = "This module already exists in the grad pad";

'''

Perhaps we can rename the variable 'editModuleDescriptor' here and also the references in the few lines below it.

'''suggestion

    EditModuleDescriptor editModuleDescriptor = new EditModuleDescriptor();

'''

'''suggestion

public static final String MESSAGE_DUPLICATE_MODULE = "This module already exists in the grad pad.";

'''

'''suggestion

/**

 * Stores the details to edit the module with. Each non-empty field value will replace the

 * corresponding field value of the module.

 */

'''

'''suggestion

/**

 * Creates and returns a {@code Module} with the details of {@code moduleToEdit}

 * edited with {@code editModuleDescriptor}.

 */

'''

I think we need to update the javadocs here 😁

'''suggestion

Deletes a Module identified using its displayed index from the grad pad.

'''

Perhaps we can use camelCase for the property names? JSON properties are basically hashmap keys; so just like how we wouldn't use multiple words (with spaces) in a key or variable name (altho JS/JSON allows), maybe we can use moduleCode or module_code instead?

'''suggestion

 * Constructs a {@code JsonSerializablegradPad} with the given modules.

'''

Not a suggestion/request for change, but I was just wondering why the original code does this redundant check here when it could just call gradPad's 'setModules' fxn, which already uses 'UniqueModuleList' to ensure all modules are unique.

I just put this comment here first in case we want to improve code quality next time.

'''suggestion

List gradPadModules = modules.stream().map(JsonAdaptedModule::toModelType).collect(Collectors.toList());

try {

gradpad.setModules(gradPadModules);

} catch (DuplicateModuleException ex) {

throw new IllegalValueException(MESSAGE_DUPLICATE_MODULE);

}

'''

I think we need to rename the file also 😁

I think this test assertion is the same as the one a few lines above 😁

Since it's supposed to include other valid values, perhaps we can do:

'''suggestion

    userInput = targetIndex.getOneBased() + CODE_DESC_CS2103T +  INVALID_CREDITS_DESC + CREDITS_DESC_CS3216;

    descriptor = new EditModuleDescriptorBuilder().withModuleCode(VALID_CODE_CS2103T).withModularCredits(VALID_CREDITS_CS3216).build();

'''

Nit: the test is perfectly fine but maybe we can replace the dummy keywords with module names

The 'lastModule' is already CS3216 right HAHA so we don't need to "edit" it with the same code and credits again

'''suggestion

    Module editedModule = moduleInList

            .withTags(VALID_TAG_CORE).build();

'''

😁 I think the re-naming of the variable was missed out

'''suggestion

A utility class to help with building EditModuleDescriptor objects.

'''

'''suggestion

 * Parses the {@code tags} into a {@code Set<tag cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf">} and set it to the {@code EditModuleDescriptor}

'''

nit: typo

This module is valid HAHA

It's why the test for invalid module in JsonSerializableGradPadTest isn't passing

Updated these 2 badges so that when you click on them, they take you to our repo's CI and codecov pages respectively:

Let me know if this regex is fine cos I'm not too sure what kind of module codes we support

I left the contents unchanged for now as I only needed the tags as defaults

I added module field templates here and left the rest of the templates alone. We can remove at the end if nobody else needs them

I only added CS2103T and CS3216 as new modules in this class and left everything else alone first. Likewise, if nobody else needs them, we can remove them at the end

Ui looks great but I think the website says it has to be the ard the same size as the original. Right now it's too big:

Feel free to resize, update the PR, and merge in your own time! 💯

I think this issue is too big and general? Maybe each person can create an issue specific to their work e.g. my issue should be "Add feature list and command summary to user guide" since I'm doing first page of our google docs

small issue maybe it should be example instead of examples?

same issue here

there seem to be some inconsistency here. do you mean JOB_TITLES instead?

do you mean job_title

I think what Shawn said makes sense. It might be better to stick to terms that are already established eg InternshipStatus. If I am not wrong does the status class exist only for Internship? If that is the case, maybe you can consider shifting date to the Internship object instead and as such, there won't be any ambiguity in the naming.

small typo

might want to maintain consistency with other files where you leave a line at line 9 for this class.

there shouldn't be a return

I realized that some javadocs there's full stop while some there aren't.

If most to all parser requires these static final int variables, we could shift it to avoid duplicates. However, I am not sure if it is apt to shift it to an interface. Can consider having a Util class for it.

the parameter should have a space

I am wondering if since the Internship has been categorized into InternshipCliSyntax, perhaps this could be also categorized into ItemCliSyntax, or were u planning to convert this class into that in the future?

hmm i am not sure which "style" we want to stick to but shawn has another way of writing this InternHunter detects an error in the input format . We might want to stick to one way in order for our DG to look cohesive.

should use

Would it be weird if it is "viewing" the profile page? To be cohesive to our DG, You must be on the **profile** page ... sounds more cohesive to our precondition

an exception should be only thrown when it is like not within the programmer's control? like missing files or user input. Since it is a controlled environment, using assertion is more apt. It's from oracle java assert.

Small issue, can put /todo for future.

not totally sure how regex works but from the description are you rejecting # and +?

do remember to add in javadocs in the future

do include the @throws.

do remember to add in javadocs in the future

just a suggestion, @line 38-39

from GitHub this line looks quite long, you can consider having

boolean b1 = ...

boolean b2 = ...

if (b1 || b2)

oh, but I think keane/shawn side is allowing the use of those though! it might be confusing for the user ? can either @keanecjy @shawn-nyk confirm this?

@throws missing

InternshipTitle instead

might want to change the variable name for clarity

Internship titles should only ...

might want to change it to INTERNSHIP_TITLE_DISPLAY_NAME

just a minor question but is there a need for this if block? I think we could just shift it into the if-else block. However, if this is a better coding standard, then I think we can stick to this.

oh if this the case, I believe the current code is a better option

Yup, It might be more consistent to change the type to category.

Thanks, @keanecjy will add the missing information. Maybe it might be more apt for us to put it as "what our app manages" under introduction

Yup we could keep it simple currently and once we merge them in the future, we could include those extensions.

perhaps once we merge the similar use cases together, we can include more extensions to it!

hmmm, I am not very sure if it should be an exception instead. I was following the assert from oracle java.

'''

The absence of a default case typically indicates that a programmer believes that one of the cases

will always be executed. The assumption that a particular variable will have one of a small number

of values is an invariant that should be checked with an assertion.

'''

Have fixed the java docs issues, thanks.

I will resolve this conversation since we have concluded it.

True I am not sure about which is a more appropriate wording.

Thanks for the spot, will update it.

Thanks for the clarification Shawn, what I meant as this naming is, Company & Application to have the same Information Width. I will do the relevant changes to make it clearer.

Not very sure how to resolve this portion. Maybe I should a raw type parameterized instead?

yup I think I should change this to setRequirements

yup I think I should change this to setIndustries

LGTM

#87

LGTM

If we are planning to create other classes in replacement of person and address book, I don't think we need to change the Javadoc yet?

#48

#99

Hmm. The dilemma now is that this is just a proof of concept and we may be abstracting common logic down the line and scrapping all these, so overwriting Javadocs now may redundant. The danger is that if the code is kept and the comments are missed in the future. What is the team's opinion on this? @ZoroarkDarkrai @keanecjy @seanjyjy @shawn-nyk

I agree that overwriting Javadocs now may be redundant, especially because our code can change over time up until the final version. At the same time, I also agree that we may miss them in the future, and doing a one-time run-through of all our code at the end may be very tedious and error-prone. As such, how about we do something like: every time we create new bits of code that require Javadocs, or we modify code that already has Javadocs attached to them, we place a standardized comment, such as TODO: Javadocs above the code we need to write/edit Javadocs for? That way we get the syntax highlighting that IntelliJ provides for the TODO keyword, and we can also search for this standardised comment throughout the code base to hone in on where we'll eventually need to write/edit Javadocs when the time comes. Any thoughts on this?

I think this is a great idea what do the rest think?

#104

#100

LGTM good job.

Hi, Fidella I think since you approved these changes I will just merge it in? Need this code currently

LGTM good job.

Can add the todo or javadocs and it is good to merge.

Which file/line are you referring to?

DeleteProfleCommand class and maybe its constructor? incase forget in the future

Will merge this first

LGTM

Besides the point that @keanecjy has raised, I believe it is good to merge after it is resolved.

LGTM

the link seems to be still broken. From the html file side

the link seems to be still broken. From the html file side

Sorry my mistake

LGTM

This was following the discussion yesterday, there is no view command for profile.

Maybe it will be more consistent to change type to category?

Also, I think this was left out in the issues, but the switch command can be added too

Do you think it is good to be overly specific about the type of error? If so then there are many more extensions to include such as inputting an index that is not a number for example.

I am unsure about this myself but what if there are no items in the list? what kind of interactions can the user and system have? I know your preconditions addresses that but do we have to consider the case where you call edit and there are no items in the list?

Really minor a UI not an UI

Should be an exception instead?

Should be an exception instead since you are not testing an assumption?

Should be an exception instead, since exceptional case

The other JD comments are in the other PR, you can ignore them for now if you want unless you it is finalized

Sure we can have a common package for the fields within model package but for unique fields with own regex, keep them in the feature's package.

Minor nit, should be Sets in the Javadoc

Same thing here. Minor nit, Should be "Sets"

Same thing here -> Sets

Change -> Changes

Same minor nit Set -> Sets

Set->Sets

There are 2 more that require changing below:

Initialized
set

Javadoc comments below as well for set.

Perhaps its more appropriate a throw an exception here since assertions are mainly to check for precondition and post condition of function execution and testing assumptions. This usage fits neither and is more of an exceptional case?

Same issue here ? You are not testing assumption here, exception better?

I think you may be missing a line break here?

The profile command is updated with [t/TITLE] and [d/DESCRIPTORS]...

I think you have have missed out deleting this UC05

Is this supposed to switch the tabs? It doesn't do that at the moment.

okay! I tested on my add and delete commands but it doesn't do anything yet. Is there more to the switching besides setting the model state which I have missed from your implementation? I think you may have to return the multiarg CommandResult as there is a boolean arg to indicate whether to switch ?

You may want to reduce depth of nesting here? I get that you need three loops here maybe if you using streams can help with that? Or 2 other functions that handles each loop and use boolean flags to terminate the final loop?

What happens with the internships don't match? Maybe I am mistaken here but it seems like you just let it pass and create the model out of the unmatching internships instead of an starting an empty list for company and application?

Do you think it would be better to shift this logic (LOC:91>) to this own file,? Main app is starting to become too big?

Maybe it would be more appropriate to include an assertion or throw an exception here if this is not expected to be called since its harder to debug with null?

I have updated the Descriptor class with its own regex, does this affect the storage in anyway?

Is there a reason why this does not return a jsonadapted profile item?

Initializes?

You can ignore this first. The content page can be changed after all the headers are in.

nvm, its good to merge, I have updated the contents page to link to my headers.

Okay, will update!

yes. These did not have preconditions, I'll remove them.

True, I should remove this, I think the only precondition then is that the system is running which is not very meaningful in this case.

Okay will make the change!

sounds good!

Yes, there will be multiple descriptors which can be added similarly to tags.

I think this is the work of IntelliJ auto injecting imports. I will move it.

I was going for a primitive implementation for now, but since the types are fixed, I think enum is good solution too.

I stopped changing this because I was unsure if we need 2 levels of inequality for profile items.

I have not created exceptions yet so this is a placeholder

I didn't change this yet since this was the original file but I suppose like it can be changed now too.

Sounds good! I was thinking of doing so after the other parsers are done since for example the EditParser for the InternshipCommand may want to abstract 3 items (comindex, index ,prefixes)? So it depends on implementation of the parsers first then after which we deal with abstraction for common vars.

Yep, some of initial JD do not have full stops too.

I'll update this! thanks for the catch

@seanjyjy yeah I was thinking you could always type c sharp or cpp for those cases .

@keanecjy Good point, I am ambivalent about this. But could the same be said for Names as well, maybe we can consider a char limit for that?

There were changes in the branch, need to resolve some conflicts! @seanjyjy

I noticed that many of the javadocs used "person" and "address book", should we change it now? or leave it for later?

Hmm. The dilemma now is that this is just a proof of concept and we may be abstracting common logic down the line and scrapping all these, so overwriting Javadocs now may redundant. The danger is that if the code is kept and the comments are missed in the future. What is the team's opinion on this? @ZoroarkDarkrai @keanecjy @seanjyjy @shawn-nyk

Yup I think that there isn't a need to concern about javadocs / comments as of now as they can be easily fixed at any point in time. @orzymandias I stopped editing the List portions as I noticed that there were too much code duplication with your pr. Do we plan to either extend an Item class or use generics for our list?

I agree. I think there is unnecessary duplication for collections, generics is a good idea for collections but one issue is with throwing custom exceptions for different types of items within the generic list. Should we just have DuplicateItemException then something like throw new DuplicateItemException(T.getDuplicateExceptionMsg())?

ecessary duplication for collections, generics is a good idea for collections but one issue is with throwing custom exceptions for different types of items within the generic list. Should we just have DuplicateItemException then something like throw new DuplicateItemException(T.getDuplicateExceptionMsg())?

I don't think this works because we are not able to access methods in java generics. I figured since we all are classifying our classes as Item, we can have an Item interface to contain all the methods similar methods like isSameItem and likewise do throw new DuplicateItemException(item.getItemName())?

Sounds good.

Overall looks great, just some nits to fix.

Just a side question as to would it be confusing as AddCommandParserWrapper is essentially Similiar to DeleteCommandParser, however, they have different naming syntax.

Is this implementation temporary in order to allow ab3 to function properly?

For AddCommandParserWrapper each item has its own set of parsing rules which may require their own individual parsers so the AddCommandParserWrapper is only responsible for funneling the appropriate item prefix to its add parser. The thing about delete command parser is that there is lesser parsing involved except for extracting out the index to be deleted. With the exception of internship which has an additional index required, all the other commands just need the index extracted to be passed to their delete commands. So I am not sure there is a need a wrapper in that case.

As for consistency I think once AddCommandParser can be safely removed, we can rename AddCommandParserWrapper to AddCommandParser.

Just some conflicts to resolve and it will be good to merge.

Can add the todo or javadocs and it is good to merge.

Which file/line are you referring to?

Shouldn't example usage (below) be updated to include the IC field as well?

Shouldn't a javadoc comment be included similar to the other models?

Shouldn't there be a multiple ICs test?

Shouldn't there be a missing IC prefix test?

Shouldn't there be an invalid IC test?

Shouldn't parse_oneFieldSpecified_success and parse_multipleRepeatedFields_acceptsLast also have tests for IC?

Shouldn't there be tests for null and invalid IC?

Shouldn't it be BloodType instead of weight?

If you refer to the javadoc for the other methods, shouldn't it be @code String BloodType and @code bloodType?

Shouldn't it be bloodtype instead of weight?

Shouldn't there be a description of the parameter?

Should it be Constructor instead of constructor?

Javadoc comment does not follow the coding standards

Shouldn't it be there be a space before Blood type: ?

Shouldn't it be BloodType.MESSAGE_CONSTRAINTS instead of Weight.MESSAGE_CONSTRAINTS?

Shouldn't it be blood type instead of weight?

Shouldn't all the missing field prefix test also include VALID_BLOOD_TYPE_BOB so that the tests would be consistent with each other?

Shouldn't it be blood type instead of weight?

Shouldn't method header comments start with a verb?

The class header comment is not consistent with the class header comment like Phone or Email

Shouldn't all these "N/A" be abstracted out?

Why not use the stringEmail, stringAddress... methods?

OPTIONAL_FIELD_EMPTY_MESSAGE and "N/A" has been used over multiple classes. Maybe they can all use the same constant in one of the classes?

Same as before, why not use the stringEmail, stringAddress... methods?

Is it necessary to use nested Optional?

You can use the or method so that the get*Field* method does not have to return a nested optional

You could maybe use the ifPresent method

Why not use the stringEmail, stringAddress... methods?

why not use the stringEmail, stringAddress... methods?

I think you forgot to update this part

as of v1.2 there is no feature that requires the generation of reports

interacting with GUI with a mouse* ?

Again, no reports yet

include date and time

This is not in v1.2

Don't edit the tutorial

Parameter name not changed

Good

uses '@' symbol

Comment: 32nd not 32rd

Comment needs to be updated

Looks like you forgot to change these comments as well

Comment should be day greater than 31 instead

Is it supposed to be date or Date, previously it was Phone not phone

KIV the prefix because this includes date and time

Perhaps it should be named modelDateTime

Missed this out

I personally feel that date and time should be separate, e.g. 01-01-2020 1200, we can discuss this in the team meeting tonight

Should probably be on separate lines

Should be named something more meaningful like status or taskStatus

Shall discuss whether these fields should be optional in the team meeting later tonight

Should be DATE_TIME

does this prefix make sense or do we need to revise it since its date + time now, maybe it should be due: for example

What is the purpose of this design wise? Is this to act as a null value without throwing NPE?

The rationale behind using java String#trim on Phone was to remove leading and trailing whitespaces. When considering the user's dateTime input do we need to consider white spaces inside the string as well? For e.g. "01-01-2020 1200" vs "01-01-2020 1200" if we are not going to trim it then perhaps we need to consider adding an exception for this and add the corresponding unit test.

Remove this

rename this

rename the date

added extension

These are in addremark.md not actual code, not sure if it needs to be changed

same reply as above

ok undid changes in these documents, please review again

not in this PR

changed

done

changes reverted

this method is to list out the message constraints, in order to have a nice grammatically correct message the last type of task in this String should not contain a comma after it

e.g. Acceptable types of tasks are: A, B, C. instead of Acceptable types of tasks are: A, B, C, .

excellent suggestion, it works as well and is more concise

Does not LGTM. Please include markdown for important commands or things to note of. Thanks.

Please include your portfolio under team/GabriellaTeh.md

Resolved merge conflicts and created a new pull request from a different branch

TaskTemplate instead of Task Template/Preset Task Template

Should Preset Task just be Task only?

For now maybe we should use remove the spacing between Task Template, Task Schedule, Module Schedule etc.

Probably should try to use module code instead of index

modules*

has to be an integer from 1-7*

has to be a valid positive integer. The Total number of weeks are calculated using the start and end date of the semester*

weeks*

has been completed*

Displays all tasks in the week that is specified by the user

has to be a valid positive integer. The total number of weeks are calculated using the start and end date of the semester.

Total weeks in the semester

Creates a template of tasks for the user to reuse weekly to quickly add into his/her schedule

List all task templates that were created

Creates and adds a preset task in the selected task template

has to be an integer from 1-7, 1 being Monday and 7 being Sunday

Clear all preset tasks in the selected task template

Rmb to change the capitalisation for UG

Maybe listtemplatetasks would be a beteter name

List all preset tasks in the task template

Maybe listallmodules will be a better name

Ternary ? should hour instead of hours

Should name the class name as Day.java

Refactor Days to Day

Some have full stops and some don't

Can consider using the Amount class so Price will store an Amount instead of a big decimal

😸

Can check out Hans implementation for the clear command that that passes a new service manager (expensetracker for your case)

Yup its quite referenced from the original AB3 tests

Better to merge this after everyone updates their profile in the aboutus page

Amend commit to update DG use cases with suggestions from @khoongwk

Not sure if add remark changes were accidental for this branch but would be useful to have it to serve as the description for tasks 😸

LGTM

Added some tests

Consider checking out Galvin's implementation for parse method of find command

Is it necessary to perform test after check? I believe check already includes test.

https://stackoverflow.com/questions/50104666/gradle-difference-between-test-and-check

Duplicate https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/13#discussion_r492195636

Do glossary terms need to be italicized? May need to standardize.

Can I confirm that this line will be left unchanged? Otherwise LGTM.

Ok, we will review this again towards the end of v1.2.

Can we verify that this workflow runs smoothly on our repo?

Perhaps you can push an equivalent change directly to a non-master branch first, and see what happens to the gh-pages branch.

Ok, as seen from https://github.com/AY2021S1-CS2103T-W16-3/tp/commit/65e48f7a1f38c68e643cb29f8d29e0951f06bc4c the CI is able to build and deploy GitHub Pages properly.

Suggest extracting these changes to their own commit/issue/PR as they are technically not related to the issue at hand.

See above (https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/23#discussion_r495016936)

Minor typo fixes can be left as part of this PR.

Similar to https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/7#pullrequestreview-492595656, Cygwin messes up on CRLF line endings in the scripts. Not a major problem; the hooks will work as long as the scripts are changed to LF line endings.

After discussion, changes can be left in this PR, but should be separated into different commits so that they will show up in the squashed commit upon merge.

These don't look like intentional changes 😕

https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/39#discussion_r495585791 same here

Minor typo:

'''suggestion

'''

Indentation still appears to be slightly off here compared to the previous state

'''suggestion

        xmlns:fx="http://javafx.com/fxml">

'''

'transaction' is no longer the parameter name

'''suggestion

 * Returns an add expense command string for adding the {@code expense}.

'''

This should be changed as well, my bad!

'''suggestion

 * Returns an add income command string for adding the {@code income}.

'''

Perhaps the variable name should be ft instead of ab?

Also I don't see any method signature for withTransaction(String, String). Is this a problem inherited from the original AB3?

Same as above, consider changing ab to ft.

'''suggestion

public static List<expense cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> getTypicalExpenses() {

    return getTypicalTransactions().stream()

            .map(TransactionBuilder::new)

            .map(TransactionBuilder::buildExpense)

            .collect(Collectors.toList());

}



public static List<income cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf"> getTypicalIncomes() {

    return getTypicalTransactions().stream()

            .map(TransactionBuilder::new)

            .map(TransactionBuilder::buildIncome)

            .collect(Collectors.toList());

}

'''

Might be possible to further simplify by extracting out the common parts.

Missing a blank line here

'''suggestion

        + "When on Expenses tab: Searches all expenses.\n"

'''

Should this be something like a ListOverviewCommand instead, to highlight that it is for the overview tab?

Ok, makes sense.

Perhaps getTypicalFinanceTracker().getExpenseList().size()?

By the way, it doesn't really matter here but the expected value is supposed to be the first argument to assertEquals, and the actual value comes second.

Fixed

Note that placeholders johndoe.md and johndoe.png are being removed as they are no longer needed.

It was found that the shell scripts do not produce any output unless problems are detected.

To make the pre-push hook and CI job more useful, the changes in #23 will also modify the scripts to print their status as each check is being run.

Note that while we intend to rename test methods to match the new model, we will leave the test data alone for now and revisit it in a follow-up issue.

https://github.com/AY2021S1-CS2103T-W16-3/tp/pull/39#pullrequestreview-497089729

Update the package names too to ay2021s1-cs2103-w16-3.finesse as discussed.

Edit: My bad, hyphens are illegal characters. How about ay2021s1_cs2103_w16_3.finesse as recommended here?

Recommend doing this in a separate PR. Refactoring the package would generate a large diff and make it difficult to review the model renaming.

Now that #39 has been merged, all that is left is to delete the Address class, which is incidentally one of the CS2103T tP tutorial tasks.

How should we rectify this? Fix the sample command or fix the validation?

I like your specifying: STUDENT_INDEX and CLASS_INDEX

Perhaps it is clearer to define 'index' as follows?

'''suggestion

*'STUDENT_INDEX' refers to the index number shown in the displayed student list.

*'CLASS_INDEX' refers to the index number shown in the displayed class list.

'''

Minor grammar issue, should it be 'is' and not 'are'?

'''suggestion

5a. At least one of the given indexes is invalid.

'''

Same as issue above.

'''suggestion

5a. At least one of the given indexes is invalid.

'''

Should we include test cases for UUID? Such as public void toModelType_nullUuid_throwsIllegalValueException().

Apologies, mistake on my part for not typing correctly. Should be

'''suggestion

public void toModelType_nullUuid_throwsIllegalValueException() {

'''

Otherwise, looks good to merge.

Should the tense follow the 'DeleteStudentCommand' class instead?

'''suggestion

public static final String MESSAGE_DELETE_MODULE_CLASS_SUCCESS = "Deleted Class: %1$s";

'''

Nice abstraction.

Should the filtered list be specified?

'''suggestion

 * Updates {@code model}'s {@code filteredStudents} list to show only the student at the given {@code targetIndex} in the

'''

Same as above.

'''suggestion

 * Updates {@code model}'s {@code filteredModuleClasses} list to show only the ModuleClass at the given {@code targetIndex} in the

'''

Same as above.

'''suggestion

 * Updates {@code model}'s {@code filteredModuleClasses} list to show no module classes.

'''

Perhaps naming studentIds as studentUuids would make the code clearer?

Should there be at least one class entry here? We can follow invalidClassTutorsPet.json, which has a valid student entry.

Should you insert a INVALID_STUDENT_IDS list (and test case) also? Perhaps you can use a string that has less than 4 hyphens

Nice handling of null UUIDs. However, would it be better if a try-catch block is used on UUID.fromString(uuid) instead? Then, any kinds of wrong UUIDs can be handled.

I think you missed out on this:

'''suggestion

public static final String MESSAGE_DUPLICATE_MODULE_CLASS = "Class list contains duplicate class(es).";

'''

I think you missed out on the TODO comment. Perhaps instead of importing these STUDENT_UUID_X in test cases, we can call ALICE.getUuid() from TypicalStudent class instead.

Should this for loop be placed in another method for proper SRP? The for loop above that iterates through jsonAdaptedStudent can be similarly placed in another method. Then, this toModelType() method will call these two for loop methods.

To add on, according to Java's UUID API, UUID.fromString() throws IllegalArgumentException. Perhaps you can use this alongside with your null guard clause.

I notice this try-catch block appearing in other parts of the PR. I think the proper way would be to write our own interface for UUID. But, for now, I think it is not needed.

Should you indent this line?

'''suggestion

<fx:root minHeight="600" minWidth="740" onCloseRequest="#handleExit" title="Tutor's Pet" type="javafx.stage.Stage" xmlns="http://javafx.com/javafx/11.0.1" xmlns:fx="http://javafx.com/fxml/1" cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf">

''' </fx:root>

I noticed that these 3 lines of code are repeated in above test cases. Perhaps can consider abstracting these out into a private method?

Should this be:

'''suggestion

    ModuleClass specifiedClass = showStudentsInModuleClassAtIndex(expectedModel, INDEX_FIRST_ITEM);

'''

Should this be in the same 'format' as above?

'''suggestion

    assertCommandSuccess(new ClearModuleClassCommand(),

            model, ClearModuleClassCommand.MESSAGE_SUCCESS, expectedModel);

'''

Would it be better to test against the moduleClasses directly? Since we are adding (and removing) them along with their studentUuids.

'''suggestion

    assertFalse(modelManager.hasModuleClass(CS2103T_TUTORIAL));

    assertFalse(modelManager.hasModuleClass(CS2100_LAB));

'''

Similar to above.

'''suggestion

    assertFalse(tutorsPet.hasModuleClass(CS2103T_TUTORIAL));

    assertFalse(tutorsPet.hasModuleClass(CS2100_LAB));

'''

Perhaps you can convert the for loop in ViewHistoryCommand into a method residing in StringUtil class.

This is to avoid the explicit construction of expectedMessage. For completeness, the new StringUtil method also needs to be unit tested.

Yup

@junlong4321 yup, that is right.

@dextertanyj agreed, will remove a student not in any classes.

I think that to match prefix, the e.g. would be sth like list l/people, list /stats. Please check on this.

Might want to refer to this as Virus Tracker instead of AddressBook.

Might consider changing to 'Clears all entries from Virus Tracker' to match description in line 42.

It appears that you have added some new functionality here regarding some statistics. Can these be used already?

Considering the nature of this test, it might be best that the method updateFilteredLocationList is completely removed from the working program since it is not expected to be called.

Additionally, on this aspect, I realised that updateFilteredLocationList is a method newly added to Model and ModelManager. Considering that, may I check why is the method added if it is not intended to be called as seen from this test?

I understand now. Thank you for the clarification.

To keep the code clean, I feel that this line should either be uncommented or removed.

Ids are abstracted away from the user. Users should not be aware of Ids, and thus should not be given the opportunity to change the id of a person. I feel that thus editpersondescriptor should not have an id field and id is automatically retrieved from the person that is being edited.

Some additional tests can be conducted in this class. Such as tests for adding unidentifiable locations and tests for setting location lists which dont fulfill the identifiable property. You may refer to #118 UniqueLocationListTest for reference.

One more test was missed.

To abide by SLAP principle, consider abstracting out different parts into a separate methoda in the same class.

For e.g. line 50 to 62 can be abstracted into a separate method

The "..." seems to imply that the command can accept multiple personids. Is that intended?

You have made tests that dealt with the edge cases. It would be good to have additional tests that prove the success of the command. In this case, it would be giving an infected person and confirming that the generated results are the expected results.

Could this perhaps be shifted to info handler class? This can help to keep calculation and information handling methods consolidated in one place.

This code is quite long. May I suggest extracting part of it into another method? This is a suggestion in accordance to single line of abstraction.

You have added tests capturing failing conditions. May I suggest tests that confirm success conditions as well?

Is this possibly a spelling error? Form or From?

Perhaps some documentation or name changing could be done? Its hard for me to directly understand the significance between 60 percent and the visits chosen.

This test feels like it might be more appropriate elsewhere. This is a class for tests involving location as itself. Perhaps this test might belong to a info handler or for a class that directly implements this.

Perhaps these methods could be shifted to info handler class? This can help to consolidate all information handling methods to one class.

Suggest using a comment block instead of multiple single line comments.

A comment block looks like this.

some comment

more comments

Suggest putting both comments in 1 line rather than having multiple single line comments.

This method seems very similar to the above method getTypicalLocations. I feel that since both methods are used in test cases, naming this method getLocationsForTest doesnt differentiate the purpose of the 2 methods. Perhaps naming it as getUnorderedTypicalLocations() would be better?

This method seems very similar to the above method getTypicalLocationBook. I feel that since both methods are used in test cases, naming this method getTypicalLocationBookForTest doesnt differentiate the purpose of the 2 methods. Perhaps naming it as getUnorderedTypicalLocationBook() would be better?

identical visits here, resulting in error.

Spelling error is intentional for the invalid person.

Have added additional assertions.

Have changed the method name

Thanks for the catch! I have updated it.

This copy of valid visit is hard coded, by using the visit builder class. The hard coded values are retrieved using the getOneBased method. Valid visit has no direct reference to filtered visit at all.

The gist of this test is that my model's filtered lists for person and location currently store only 1 location with id 3 and 1 person with id 4. This is done using the showLocationAtIndex, showPersonAtIndex methods above. Im giving the model input using index 1 and index 1 for person and location, because the list user sees contains only 1 element. After processing however, I am expecting the visit to point to ids 3 and 4 respectively. This is done by comparing the final visit from the visit list with the expected visit created using valid visit.

isnt this repeated with #19?

isnt this repeated with #18?

Pull Request Approval:

Before approving any pull requests, please ensure the following is done.

Eyeballing the code for any possible logic or other errors. This is also to familiarise yourself with implementation of the system.
Pull the pull request down into a local branch and give the features a test run. This is to check whether it actually works since eyeballing code may not be fool proof in ensuring functionality.

In addition, you might want to update the details of the pull request if it does indeed include some functionality involving the daily statistics. This is on the assumption that it is included.

Pull request was merged using Admin powers without going through 2 approvals.

However, noted that I would have given approval after the recent commits, so no harm done. Please be careful in the future though.

Thanks for the review. I have improved the Pull Request according to your comments.

Suggest to merge visit id fix PR first #136 and see if any of the tests crash for this particular PR.

If none crash, I will automatically give my approval for merging this.

Shall be closed as participation has already been counted and PR is not relevant for VirusTracker

Completed by #126

Hi everyone, please help run a few test runs with the application.

Pull from this branch into a local branch and run a few methods that you created for the application. This can help confirm that the application has not bugged out or lost any functionality during refactoring.

I think "module" here should be "modules" instead.

"Lists all modules provided by the university"

Unless 'n/' command is taken, I think we should use "n/" instead of "un/", it seems more intuitive for the user to add his/her own name as "name" rather than "username"

Should we allow the user the flexibility of determining start and end date? Isn't it better to only allow the user to specify the academic year, and we will provide the start and end date for the user. I think this would make it more convenient for the user.

Similar concern to my above comment, whether we should use "n" instead, as well as whether we should allow the user to have control over the start and end date semester.

'DAY' has* instead of have. Also, I think it'll be good to capitalize first letter of the days Monday and Sunday:)

Similarly, "WEEK" has*.

Additionally, "Total number of weekS".

If we do decide that user shouldn't be able to select start and end dates, then the total number of weeks will be pre-determined already.

Same grammatical error as above comment

There's an additional 'e' here in "edituseretask"

task ID example given here is not really consistent with the sample command (line 248) which uses "id/usrta001". Additionally, the example command here uses "dy/3", I think it should be "da/3".

I feel that the command could be better named, for example using "edittemplatename" instead. Although its a slightly longer command, however it gives more clarity than "edittemplate" as to what the user is editing. "edittemplate" gives off the impression that I can edit preset user tasks in the task template?

Why is this named "listtasktemplate" instead of "listtemplate" for consistency with the above commands "edittemplate" and "createtemplate"?

Grammatical error:

"total tasks for that day exceeds"

Similar grammatical error for "exceed"

I don't you need "pti/1" in a cleartemplate command.

Grammar: I think it should be "List all the task templates that were created" instead

Is MODULEINDEX here a list index like "3"/"5", or would it be a module code like "CS2103T"?

I think deletetemplate here should have a "tti/TASKTEMPLATEINDEX"

In the e.g given, cleartemplate shouldn't have "pti/1"

I think we are going to omit MyModules from the architecture, we will be using ModuleSchedule instead, so this use case should be View ModuleSchedule instead

Should we follow the three part format for naming of test methods?

Likewise for the naming of the test method

Similarly for the naming of test method

Same here for the naming of test method

With this implementation of the parsing of user input, what happens if the user inputs something like:

addexp ... f/for now no OR addexp ... f/think no.

Although in an actual use scenario this is very unlikely to, or will never happen, but would the parser still allow the creation of the expense object in this scenario?

GrAB3 should be SuperSalon

I think you can add system and actor here and for all other use cases

Okay thanks! I'll change it

Thanks for the comments @khoongwk @yanlynnnnn , I have made the changes as requested!

Thanks for the comments @khoongwk @yanlynnnnn, I have made the changes as requested!

Perhaps can add a description saying that this is the constructor?

Should be project book instead of address book.

Same here too as above comment.

Perhaps there should be a whitespace after the slashes?

Yup think it will better to describe that it will construct a new project, just like the javadocs in other classes.

Will it be better to give an example of how the ISO8601 time format looks like in the error message? A user might not know what this format is.

Should have a new line before the param.

Think the persons here should be timers instead.

Should have a new line after the description. Same for the 2 methods above.

Newline after description.

Extra line here should be removed.

Extra asterisk here should be removed.

Newline after description.

Newline before method.

Should we remove all the navigation commands first? Since they are not yet implemented.

Description has been changed to an optional field.

Should both name and description be optional fields here?

I think we can indicate [coming soon] for this.

I'm not sure if this section should be done by @boundtotheearth, since he implemented this and according to the tP instructions, it is recommended that features in the user guide should be written by the person who implemented it.

Similarly, should we remove this first or indicate it as [coming soon]?

Same as above, for Create and Update, some of the fields are/are now optional.

Not sure if it would be better to rename this as 'Editing a project'? It might be more clear that the name of the feature is the same as the command.

I think it should be. Because it doesn't make sense for the user to have to enter something for the name when they are not intending to change it. I think we might have made a mistake when writing this section of the user guide.

So the edit command will be like the addressbook's, where all fields except index are optional, but at least one of the optional fields must be provided.

Same case for **Note:** here as above.

Seems like markdown isn't converting **Note:** over here to bold. I think it's because it's embedded inside HTML. Try using Note: instead.

I think the asterisks here are causing Markdown to not format the table correctly.

Ok yup I agree with this. Will make the changes.

I feel that it is ok to have this method as well as the hasDescription() method in the Project class, as a Project should be aware of the contents of its own fields and I can directly ask a Project whether it possesses these fields.

However, what you said sounds fine to me too. What do the others think?

@seanjyjy is going to redo this part(?) so should I ignore this?

Could be confusing saying job name because the format actually uses the position of internship in the company?

I don't think these are the right precondition and guarantees?

just a nitpick but I noticed that other use cases use list user profile items instead user profile item?

shouldn't precondition be the state of the system and not the user?

Another nitpick, maybe it's better to use InternHunter rather than the app? As the next points use InternHunter rather than the app?

I think you missed this comment?

I think you missed this comment an others below too?

should be instanceof UserProfile?

is it the user profile?

type is category right? because it can only be either education, skill, or achievement should it be an enum instead?

I assume this is a planned extension for the profile items to have tags?

should be two profile items? and several comments below

should be profileItem? and for other comments below too

wrong exceptions in this file or are they placeholders?

Is it switching to the applications tab?

I noticed you changed of a company to in a company for others

for consistency maybe use tab because we usually describe them as tabs and not pages(?) for this and others below

this is a really small nitpick but i feel the flow of the second sentence can be improved? just feel a bit weird having the if any in the middle.

maybe something like any applications with the deleted internship, will also be deleted?

is this safe when string length is <= 1? or is it guaranteed that the length > 1?

validity?

any reason why an assert instead of an exception?

CMIIW:

2 application items are equal if their internshipItem, status, and statusDate are all the same.
deepDelete uses the above equality to delete things.
ApplicationItem(internshipItem) will create a new application item with the default status and statusDate
The application with the internship we are deleting may not have the default status and statusDate?

shouldn't this be the javadoc for the method below?

isn't util just a general purpose utility class?

oh yeah, thanks for noticing 😅

Sure!

Yes, would it be better to start from empty ones? Will change it then.

Nope, the storage just need the isValidMethod

Ahh forgot to change it back, was testing. Same with the application item one. Thanks

I noticed that many of the javadocs used "person" and "address book", should we change it now? or leave it for later?

LGTM, good work! Some merge conflicts to resolve first.

Fixed!

lol i just pushed the change

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I mean the toString, equality, hashcode tests. Could use VALID_WAGE, VALID_PERIOD, etc? If it's a better practice to use "Sunday", "React Native", etc. it's fine then.

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I mean the toString, equality, hashcode tests. Could use VALID_WAGE, VALID_PERIOD, etc? If it's a better practice to use "Sunday", "React Native", etc. it's fine then.

Not too sure what's the correct which is the correct way, but I just thought that doing it this way by writing 2 strings that are different objects would be better since we will be comparing periods with different strings in the actual implementation too

Sure then! I just thought maybe using constants could make it easier to see, that creating an object with the same input will result to an equal one. Like 2 objects created with VALID_PERSON_ALICE are equals but not with one created with VALID_PERSON_BOB.

LGTM!

I think you pattern did not match our command. Here is my suggestion:

(?<type>[A-Z]\s+)?(?<commandWord>\S+)(?<arguments>.*)

Maybe we rename this class to Contact now?

Ahh I see, just add another \ before each of the current

I think this should be kept simple, for example: "Adds a contact to the app" . This applies to all of your messages

You're missing the "T " prefix before the COMMAND_WORD here and at the example a few lines below too 😄 This applies to all 5 of your commands

I think the example for time here should have hour:minute as well, we'll support that too

As discussed, I think this should return a task's to string, and view it by Id! We won't have any find command

After coding I think that all of these "notes" like things should be called "notes" with a prefix of "note/", and this field is purely optional. What do you think?

for my iP I have written quite a nice date parser here, I think you can adapt it to this part (and add more supported format too)

this comment here is quite obvious right? I think can just write it as "Parses a Remark"

As I have commented on ViewTaskCommand, this should be modified accordingly

Ah my bad, by index

yeah I know, but like I want to create a special prefix for the note-like stuff, for users to throw anything they want into it

Okay then maybe you create an issue for that?

okay sure!

okay!

the Task.Type is missing here 😄

Missing Tash.Type here too!

should we merge Location into Remark now?

Not Email right?

Just wondering, is there any better way than manually checking 5 fields to see if they are null or not? I think there is a requireNotNull method as well right?

not String right? desc has type of String but for the message it should be Description

Not Double.class.getSimpleName() right? It should be Weightage

Okay sure

Yeah but for the message you can't output like "missing a double", better to just replace it by "Weightage" (a String) since the Double.class.getName() is basically get the name of that class

My bad😢

True

Hmm I have removed this class already?

Ah okay not yet, my bad

Can you take a look at my new PR?

If that PR is good then can merge it first, so that there won't be too many commits in a PR

Okay I forgot. Actually the original code doesn't explain the returned result too (maybe too obvious?)

Yeah, since originally there were setContact and setContacts as well

the original code base doesn't have that throw as well. What do you think? (Honestly I haven't read about when we should state that this method will throw something and when we can just omit it). Anyway, will read it later on after I finish developing these 3 classes

I think we should best leave it as TrackIter (short,concise name). LOL It took me 1,5 hour to rename address book to TrackIter 😂 (And still there are a lot of leftovers)

Fixed in the latest commit

Okay lemme fix it now

When I thought on how to handle the program's logic, I found out that putting lessons and tasks in Module will make it way harder to find/delete tasks & lessons. Instead, I think that we just keep lessons and tasks in separate lists and everytime we need to find lessons or tasks of a module, we can just do a O(n) search. This will make the program's logic much simpler and less prone to errors

Yes. Is just a more detailed type of exception, so it doesn't need any implementation

It will be fixed in the next PR!

Wow I think this is a bug happend when I'm copying things

fixed!

Ah okay okay let's enable it

@simonteozw Maybe you approve this PR first then I make another PR? I just run the code formatter and it changed like 80 files

Yes, I realised it too but I forgot to raise it up during yesterday's meeting! Will send Simon another PR.

Ah I resolved it already! Forgot to close this issue

@simonteozw can you review this commit then merge it for me so that the next PR won't be so huge

Actually "commons" in the name of one of the current module too! (seedu.address.commons)

Ah that checkstyle, I think it can be disabled for now, later on when I merge the later pull request I can enable it back again! Actually we can do cherry-pick there but I haven't had time to properly learn cherry-pick

@nguyendqminh can you take a look at my message above and provide me the deadline that you will resolve this? Thank you!

@simonteozw Hey can you take another look? If it's good then you can approve it so that I can make the final PR for Lessons

@nguyendqminh Hi, any updates on this?

@nguyendqminh Can you push your commits to the PR so that the team can know your progress? Thank you!

Is it better to change this sentence to this?

"This project is based on the AddressBook-Level3 project created by the SE-EDU initiative."

As I feel like this flows better for me.

Should this be linked to our repo's actions instead of the se-edu's one?

no i mean the link not the badges. Now you are linking it to se-edu/addressbook-level3

https://github.com/AY2021S1-CS2103-T14-3/tp/actions

this

I think this line can put?

I'm a bit not sure about this. Is it we can edit the travel plan at top directory also?

dk hahaha

@jiaweiteo @jeannetoh99 @timjkong

According to @jiaweiteo , only the name should be considered as identity fields, while passport and phone should be data fields

i think its better to put getTPOList()

same for all the other TPO

Is it better to have ScrollPane here? As we might have lots of TravelPlan and the Pane cant show everything

Same as above. Maybe we put ScrollPane here

JavaDoc errors

We just need one method from Directory only? What about AccommodationList all that?

Is there a better way of doing this? Like not typecasting but instead use the polymorphism.

Why did you use assert here btw? Cos I think assert won't be run if we didnt specify it and the user probably wont run our program with assertion on. Maybe check it using if block and throw an exception? Same for all the methods below

JavaDoc error

same for line 57

JavaDoc error

thats why before i put 2 boolean method to check is which one

nah typecasting is not considered polymorphism. We have to do directory.the method straight away i think. But it might be not worth the time to think about that. So maybe can leave it like this first ba

oh okok then Logic is the one that have to make sure the directory is the correct instance?

Need, but i thought haishan is also doing this so I left out the edit and show features for her, or I should just do them?

Hmmm I don't know.

@jeannetoh99 @underthehai @timjkong

Maybe can add the person to be deleted is not found?

You mean the person is not found in the travel plan or you mean the person is missing in the input command?

I will add the case for target not found, but the error message one cause I wanted to make them generic so don't need to be too specific. The implementer of that feature can do whatever they want to the error message. What do you think?

@underthehai

I updated it. But I don't know if it works, can help me check?

My CI will pass after merging @jeannetoh99 TravelPlan class

Don't merge first after commiting changes.

I need to think about all the ReadOnlyXXX classes, if they are needed

I think I'm done for now? Hopes everything works.

And do you think we need to make sure that the travel plan inside travelPlanner needs to be updated too?

Small issue, but should this line not begin with a whitespace? (Even though the syntax highlighting does not appear here on GitHub, I believe it still works to markdown the text in this case so shouldn't be an issue!)

Should we markdown these lines with backticks as well, to put them in code format, as per the other examples?

I assume we're using Person's Name and Phone classes for now since they help guarantee certain text properties. We'll probably have to change this in the future (at least the naming, for example wage being a Phone object seems odd)? Perhaps using String for now may keep things even simpler, but I suppose this works for now!

Perhaps we could name this InternshipStatus or something along those lines instead? Do let me know if I'm mistaken, but it seems that we have a Job class that has the variables - jobTitle, companyName and industry. So this status object should be reflective of an Internship status rather than a Job status? Shall we ensure that "Job" and "Internship" are not interchangeable terms throughout our code base? Another issue that might still arise even after changing the name to InternshipStatus is that we will then have a Status and InternshipStatus class, and it may not be obvious what the difference is between the 2 in terms of their names? Would it be possible/better to make this a private enum within the Status class?

I suppose having this field means in the UI, we'll have some text like "As of: date"?

As we'll also need to add Jobs in a company object, this Job object seems to differ implementation-wise from the Job object that we may use for company objects, though they should probably be the same? For example, in a Company object, during its creation via a user command, the user can input multiple industries and jobs that a company has. This creates the Company object, along with its multiple jobs and industries. So in those Job objects in the Company object, will each of them have multiple industries tagged to them instead of just one industry as in this Job object? Also, since a Company stores Job objects, would this create a cyclical dependency if we change the Name companyName part to Company company in the future (I assume we will)? At this juncture, it seems that if Job is going to store Company and Industry, perhaps we may not even need Job objects? Perhaps we could simply use String (or Name) for jobs? This may also help to synchronize with how jobs will have to be represented in a Company object.

Sounds good to me! Shall check with the rest if they're okay too! (Sounds like in the future we could perhaps repurpose the "Person" package to become like a text-format-enforcer package of sorts?)

Ah I see, thanks for the clarification! I think the Status and ApplicationStatus names suggested work, but perhaps sticking to InternshipStatus might still be better so we don't introduce more terms for the same object (for e.g. application and internship both refer to internship applications)? So maybe swapping the current Status and InternshipStatus names will do? May have to consult the rest haha, just some suggestions!

Ah okay! My bad for missing this out, thanks for highlighting!

Should this return companyName instead?

To stick to a standardization, shall we also rename the InternshipApplication class to InternshipApplicationItem?

I believe these attributes for Company and Internship are the outdated ones?

I see we've changed the tab name from plural to singular. I was wondering if it would be odd since the user would be looking at a tab labelled "Company" but in it can be multiple companies? Likewise for "Application" as well, which to me sounds like I'll view a single application if I click on it. Small matter but perhaps we could discuss this further and see what the team thinks too!

Alright, no worries! Will merge this first and we can settle this next time.

Just to confirm, does company application refer to internship application i.e. our application class in this case?

Ah I see! Thanks for the clarification Sean!

Just tried using the same line of code for the add company command I'm working on and yep, even upon successful addition of a company, this line of code does not switch tabs. Will follow Isaac's observation and see if I can make it work for add company in the time being!

Just tried successfully on my end for add company command, regarding the switching of tabs, changing this to return new CommandResult(String.format(MESSAGE_SUCCESS, applicationItem), false, false, true); will do the trick!

This works if all our commands require a switch? At the moment that seems to be the case apart from the help and exit commands if I'm not mistaken. Also, I suppose it depends on how we want our app to behave exactly - do we want it to switch every time a command is executed even if the user is already on the right page, or do we want to switch only if the user is on the wrong page? I suppose this may not be detectable visually unless the switching of tabs introduces its own behavior (for example if switching causes the list of cards to jump to the top of the list, and not switching allows the user to stay at wherever they've scrolled to in the list, then executing a command that always switches vs. one that only switches when necessary will provide different user experiences).

Should the second argument be AddProfileCommand.MESSAGE_USAGE instead?

Should this be outside of the exceptions package?

Minor nit, but perhaps we could change this to setRequirements in the future? Just to keep all the namings consistent.

Same thing regarding the tags naming here. Unless I'm misunderstanding the use of the word "tag" in the context of this UI portion of code. Apologies if that's the case!

Perhaps this newline is an extra addition?

True, this class is indeed very similar to the Name class. However, in order to do the following:

pass in the messageConstraints to signify a different error message if need to

perhaps we can modify the Name class to, say, take in an extra argument in its constructor for the messageConstraints variable, and then the passing in of this variable could be done by the Industry class that extends the Name class and simply passes its own messageConstraints through the parent (i.e. Name's) constructor via super(). As such, we'll reduce code duplication but still maintain the existence of the Industry class. I feel that it may be necessary to have such unique classes because variables such as private final Set<Industry> industries = new HashSet<>(); would look rather odd as private final Set<Name> industries = new HashSet<>();. And ultimately, in order to "pass in the messageConstraints to signify a different error message if need to", I believe we'll still need to have some sort of Name as parent class for subclasses such as Industry, Descriptor, Requirement, etc. arrangement, since Industry is not the only class affected by this / has this characteristic.

All that said, modifying Name and abstracting all the common fields to subclass from it at this juncture would be inappropriate in the context of this PR. Would it be okay if we let this portion through first and do the necessary abstraction / refactoring after all 4 items (inclusive of this Company item) are present in our codebase, as discussed in our Tele group?

My bad, thanks for catching this!

Oh yep great idea! I've made the changes!

I can't shift it straight into the current if-else block due to the nature of the condition checking. If we want to make it all part of one block, here's an option:

'''

if (model.getTabName() != TabName.APPLICATION) {

model.setTabName(TabName.APPLICATION);

shouldSwitchTab = true;

model.setViewIndex(targetIndex);

} else if (model.getViewIndex().equals(targetIndex)) {

resultMessage = messageAlreadyViewing;

shouldSwitchDisplay = false;

} else {

model.setViewIndex(targetIndex);

}

'''

But this means 'model.setViewIndex(targetIndex)' appears twice.

Another option is to simply remove the if condition and do:

'''

if (model.getTabName() != TabName.APPLICATION) {

model.setTabName(TabName.APPLICATION);

shouldSwitchTab = true;

model.setViewIndex(targetIndex);

} else if (model.getViewIndex().equals(targetIndex)) {

resultMessage = messageAlreadyViewing;

shouldSwitchDisplay = false;

}

model.setViewIndex(targetIndex);

'''

But this means that even when the model's view index is already the index that the user inputs, we still set it anyway (unnecessary call to setViewIndex). It won't affect the program though.

Between the current code and these 2 options, which do yall think is better? Or are there other alternatives yall may have?

Hi Keane! Unfortunately that would not be possible. Variables can't be declared in an if-else block and then used outside of it due to variable scope. Even doing something as simple as:

'''

if (true) {

boolean shouldSwitchTab = false;

} else {

boolean shouldSwitchTab = false;

}

shouldSwitchTab = true; // line X

'''

Will lead to an error on line X whereby it doesn't detect that shouldSwitchTab has been initialized.

(More on it here: https://stackoverflow.com/questions/17444020/why-cant-variables-be-declared-in-an-if-statement)

As such, this already is the smallest possible scope!

To justify having these boolean vars initialized and declared where they are, we can take it that by default, we assume the user in on the appropriate tab (shouldSwitchTab = false) and not viewing the current item (shouldSwitchDisplay = true) which is why they would want to execute the view command in the first place. However, they may happen to be on the wrong tab (first if statement checks for that), or they may happen to be on the right tab and already viewing the item (the following else-if statement checks for that). That is how I would intend the code to be read!

Ah good question! I raised this in our Tele group last night, but basically how the view index works is that it takes the index from the tab that is currently in view. So for example, if a user is on say the Application tab and he is viewing the 3rd Application, if he executes view com 3, if we do not check for the tab first, then the program will simply think we're already viewing the 3rd item so it doesn't need to do anything, which is wrong of course. That said, now that you've raised this point, I believe I may be able to abstract away the first if statement, but I'll definitely need that condition check in the else-if statement that checks for if the user is on the same tab and currently viewing the indexed item. So I can minimize the tab checking but we need that one minimally.

Ah my bad! But then will we be violating this coding standard rule?

No problem, thanks for highlighting! Have made the changes!

Oh if that's the case, sure I don't mind changing it! But then again I'm not sure if our evaluators have the same understanding though? For example when we eventually evaluate our peers' work, I would probably follow the coding standard to cross-check my evaluation and if I spotted this violation in their work, I would probably just mark it as a violation since it is stated in black and white and not have to think too much. If you have the source, could you point me to where Prof Damith mentioned that we could do otherwise? Would be much appreciated!

Overall looks great. Good job. Just a few nits and it will be good to merge.

Thanks for pointing them out Sean! Have gone ahead to fix those inconsistencies across the whole doc.

Overall looks great! Just wondering if grouping the 2 stars and 3 stars together would be better? Perhaps edit an object should be considered a high priority as well? Otherwise LGTM

Ok! Made the edit commands high priority, which solved the 2/3 star grouping issue as well!

Hmm. The dilemma now is that this is just a proof of concept and we may be abstracting common logic down the line and scrapping all these, so overwriting Javadocs now may redundant. The danger is that if the code is kept and the comments are missed in the future. What is the team's opinion on this? @ZoroarkDarkrai @keanecjy @seanjyjy @shawn-nyk

I agree that overwriting Javadocs now may be redundant, especially because our code can change over time up until the final version. At the same time, I also agree that we may miss them in the future, and doing a one-time run-through of all our code at the end may be very tedious and error-prone. As such, how about we do something like: every time we create new bits of code that require Javadocs, or we modify code that already has Javadocs attached to them, we place a standardized comment, such as TODO: Javadocs above the code we need to write/edit Javadocs for? That way we get the syntax highlighting that IntelliJ provides for the TODO keyword, and we can also search for this standardised comment throughout the code base to hone in on where we'll eventually need to write/edit Javadocs when the time comes. Any thoughts on this?

Great work! I noticed you helped to check all our commands to ensure consistency in the messages too. Just need to fix some checkstyle issue before merging can be done.

Thanks Keane! And oops my bad, made some last minute changes and pushed after I first made the PR (introduced some checkstyle errors in the process). I've made a few other further changes to remove some hard-coded messages and make our add commands' success messages similar in execution to that of delete commands (calling upon the Messages class). For your vetting!

Besides the point that @keanecjy has raised, I believe it is good to merge after it is resolved.

Thanks Sean! Have made the changes!

To explain this code:

Line 43 executes a function that creates delete int commands and executes them, causing the model's internal tab name to be set to company. Thus, before that line is executed, we need to save the current actual tab the user is on, which is done in line 40. Since we are checking against this saved tab instead of the model's current tab (would become company because of line 43), we cannot use the CommandUtil.getCommandResult() method.

Maybe you can use an empty line instead of using  

There is an extra   at the end, which will increase the gap between this line and the Acknowledgement. Is it intended?

Should there be an extra white line here?

Maybe there shouldn't be a < here?

I think you broke some of my parts. You can review this markdown file again to find out more.

Before

After

I have night mode enabled so it is more obvious.

There's still something wrong here.

Did you change the package name to fileAddress?

Shouldn't it still be tag?

Maybe can replace person with tag in the message as well.

Should the person package be renamed to tag?

This may not work later because of the FileAddress field. Maybe add a //Todo here for reminder.

JsonAdaptedTag was used to store tags for person. For now shouldn't it be deprecated since we do not have that functionality?

Is it really person details?

Is this println for debugging?

Should this be "Tags list contains duplicate tag(s)."?

Is this for debugging?

This may not work. But for now it's okay.

For now it's okay. This will be looked at again by the Logic team.

Are these tests no longer working?

Extra spaces here.

Are these 3 variables no longer independent Ui parts?

// Independent Ui parts residing in this Ui container

Maybe the name of the variables here could be changed?

Roger that

We need to change all the entities in testUtil.

It's not a typo, it's a real word =)

conform

/kənˈfɔːm/

verb

comply with rules, standards, or laws.

I find that without "\n" the format is very hard to read, so I added them.

That's going to be more readable in my opinion.

Right now the tag command is like this. You cannot distinguish tag name and file address easily.

Ok will change that

test

Resolve #36

Hi Heinrich, could you make a stub so that we can use it to work on Logic? Thanks.

Yes! I think that will be good.

Just a small grammar error. I think it should be students' and not student's?

Maybe it will be nice to change tutees to students? Since we are using students at all other places.

Perhaps it will be better to remove contact since users can edit tags as well. And tags may not be contact information.

Perhaps it will be great to include the format here as well?

'''suggestion

List Students in Class | 'list students /by [n/CLASS_NAME] [m/MODULE_CODE]'
e.g., 'list students /by n/Tutorial T10 m/CS2013T'

'''

I think you forgot to change the example?

'''suggestion

List Students in a Class | 'list-students c/INDEX'
e.g., 'list-students c/3'

'''

Nice! 👍

I like this. But i think we need to update the examples in user guide since users don't have to type @ now

Should there be a space before comma?

I don't think there should be address?

'''suggestion

    // Keywords match telegram and email, but does not match name

    predicate = new NameContainsKeywordsPredicate(Arrays.asList("12345", "alice@email.com"));

'''

'''suggestion

 * Returns an {@code TutorsPet} with all the typical students.

'''

Good catch!

I think this should change to 'AddModuleClassCommand'?

'''suggestion

 * Parses the given {@code String} of arguments in the context of the AddModuleClassCommand

 * and returns an AddModuleClassCommand object for execution.

'''

Is it possible to change 'Node' to 'Label'?

'''suggestion

 * Creates a {@code Label} with the given {@code Tag} details.

 */

public Label createTag(Tag tag) {

'''

Or alternatively change the 'Label' in JavaDoc to 'Node'?

I think there is a missing space.

'''suggestion

-fx-background-color: derive(#7392b7, 10%);

'''

Is it An UI or a UI ?

Not sure if we should use An UI.

Is there an extra semicolon?

Great job for finding so many missing full stops in comments! 👍

There is an extra space.

'''suggestion

<!-- Checks that every class declaration is followed by an empty line. -->

'''

I just realized that we forgot to write javadoc for getModuleClassList().

Perhaps this should change to 'different objects' ?

'''suggestion

    // different objects -> returns false

'''

Perhaps can add in a test for multiple student indexes/class indexes?

Perhaps this should start with capital letter?

'''suggestion

    // Calls #setStyleToDefault() whenever there is a change to the text of the command box.

'''

Perhaps it will be better to rename key to student?

agree with this

Yep works on mine too

Typo on module

Change to "A contact can have multiple tags."

Can the usage of dots be consistent? Also, add articles (e.g. an assignment) and change 'the' to 'a'

Change to 'a user', 'the user', and 'opens'

No need to use 'the'

Erase 'the'

Should there be a space here?

Not a department

Shouldn't this be different department?

Not a department

Change to Information Systems

Not a department

Change to Information Systems

Lowercase D

Should this be duplicate modules?

Perhaps change to personsCs1101s to adhere to the coding standard?

Change naming to adhere to coding standard

I agree, might be mistaken for Name in Person

I think it is a good idea, it is more descriptive (and has the same format with ModuleName)

Sounds like a good idea

Maybe Priority Queue for room search should leave it for further version? We could use a arraylist to found the first empty room probably? B'coz priority may need constant update to R/W whenever a room state is changed

I think "next" could be a little vague, coz the user may not know which room they are in, maybe "finds the first free room" could be better?

just minor gramma, "rooms"

good code quality improvement!

I am not so sure why there's a "-", so is it that the user will specifically put an "-" or is it an invalid input

I think this method should be changed with a name getValidpatient and should return a patient as the name suggests. isValidPatient is like a boolean method and should only return a boolean

Mabybe using a switch could be better for readability?

Nice to have an observablelist for getroom

maybe can change to: "there are no room in the app yet, please define no of rooms"

really nice to change to 4.0 --> 3.10

I think it's a bit unclear about the change of room number, is it that room 3 doesn't exist before change or room 3 already exist and we move all patient from room 1 to room 3 and now room 3 has all patient from room 1(previously) and 3?

I think it's better to break the sentence After the fullstop

Just small gramma "AssignedToRoom"

This sounds a bit weird to me. Maybe change it to "All patients are listed" or " list all patients"

I think later when you complete the searchroom rmb to remove the "%1$s" as it's a list of string

Good change to modifiableRoomList as a diff to unmodifaibalbeRoomList

good assertion! If the targetroom is null, it will be prehandled by the commandexception

I think instead of letting the model to handle deleteTask, the deletetaskcommand class should handle it instead for abstraction

Maybe add a success case for test?

oh then that's ok I guess

hmm I think the same patient object should still have the same temperature

Nice job to separate to more cases!

remember to remove the system.out.print after doing the "actual" testing

yup! SearchPatientDescriptor is a better name

will focus on the style!

yup, can isolate it to be another method

yup, can change to "search a patient or a list of patient ..."

sure

sure!

This is really a good idea! Thanks for bringing it up

YUP!

Hmm.. I feel both method name is ok

Definitely true

I will change it to "Search a patient or a list of patients with specific criteria"

sure!

good sugguestion!

The main reason I put this is just because I feel that it is not necessary to output a list when there is only one item

Yup! Thanks for the advice!

the substring(2) is to substract away the header ("n/")

I think getPreamble will work just fine!

actually I think getPreamble cannot work since the command is searchpatient n/alex, so there isn't a preamble for the command for now

Sure!

Yup, will take note of the gramma

I think as of now, we will save the UI for v1.3 for task, so v1.2 we will juz display the result in the bot's response

I will add more success test case in v1.3

yup

I am not sure whether we need to leave a line also, but thanks for the "s" reminder!

Sure!

Might want to remove this line since everyone else also did so

You may want to add a (written by: AUTHOR) statement before the part that you wrote because I think they want to see who wrote what.

Whoops, I had changed Year to only allow digits, so this might fling an exception since it's expecting only "4"

Just a minor nitpick for neatness, but I think these 2 lines could be on the same level of indentation as line 64.

Since year is just a String now, toString() could just return year itself. But this is also really minor.

Perhaps you could change "person" to "student".

I think you can remove these 2 lines aha

Should this be ClassVenue.MESSAGE_CONSTRAINTS?

Should this be ClassTime.MESSAGE_CONSTRAINTS?

Should this be Fee.MESSAGE_CONSTRAINTS?

Should this be PaymentDate.MESSAGE_CONSTRAINTS?

Why is there a need for this? I believe it was the same way in the Tag implementation in AB3, since when these are put together there is another module with its method to put a space between tags.

Will you be adding them withClassVenue/ClassTime/Fee/PaymentDate for Bob?

Whoops, I actually meant the space at the back of the closing square bracket.

Perhaps you could test for multiple predicates too.

One suggestion would be to create your school/year predicate too and then create a FindStudentDescriptor from it, then have a method in testutils.StudentUtils that converts it into "n/... s/... y/..." form, similar to how it is done in parseCommand_edit(). The outcome would be:

FindCommand.COMMAND_WORD + " " + StudentUtils.getFindCommandDetails()

Additionally, if you are using prefixes, you might want to refer to them by how they are defined in CliSyntax instead of magic literals.

Year is actually a String now that accepts alphanumeric characters. Hence I think we should relax the requirements to be one where it returns true if student.getYear().year contains the keyword (e.g. year could be a String and it is matched using StringUtils::containsWordIgnoreCase).

That way, if you search "4" you could get those who put "Sec 4", "sec 4" or "Secondary 4" if the user is inconsistent.

It also means you will be less likely to land an IllegalArgumentException if the user parses an invalid string since right now FindCommandParser currently has to call Year#new to construct this.

What is the equals method used for?

Making the Lists public final still means you can add keywords to the list at will, you just can't reassign keywords to a new List object. Is that what you mean to do?

does this mean that if you search "sec 3" you get all that either match "sec" or match "3"? (e.g. you get sec 1-4 and p3, s3)

Yeah I think your current implementation is fine then

Would it be more helpful if we made it look only for words that contain all the keywords (even incomplete ones)? (i.e. AND search)

For example it would be like:

String year = student.getYear().year.toLowerCase();

return keywords.stream().map(String::toLowerCase).allMatch(year::contains);

Looks good! Perhaps one suggestion would be to call editAdminDescriptor::isAnyFieldEdited so we can just create the new student using student.toEdit().getAdmin().

Really damn minor note but you could probably just use Optional.ofNullable(details).map(Collections::unmodifiableSet)

add - Alex/Hogan

edit - Vaishak

find - Ying Gao/Choon Siong

After we merge this we should merge v1.2 branch into master already

Nabei I test all the methods this bot still say less coverage

My concern is that IP, IB, MOE mainstream tracks have different naming in their levels. Furthermore, there could be other level naming, for eg a tutor teaching music or arts may have different levels naming for their students.

I think this is not too bad a problem since our primary target was pri/sec/jc tutors so if we frame our target audience that way we still can meet their requirements

Looks like I will have to wait for #72 to be merged before I can pass these test cases

See #53

Functionality already exists

Basically what we've been doing all along

Maybe can add the person to be deleted is not found?

Same for accommodation and activity use cases also

So its not the command format but the error is missing person/accommodation/activity

Should we handle duplicate person/accommodation/activity when adding?

Need add use cases of edit function? Or too long already

Think the command is called 'show' instead?

I think can change address to location class? Since our user guide its location class. Can just import from activity.

Should be setAccommodation instead

Exception class shld be accommodation, same for the next one

unable to find specified accommodation

duplicate accommodation.

Should passport and mobile number be a data field since its a tag? Not compulsory for users to input.

If phone and passport is not identity fields, it shldnt be here alr

we shld restrict phone number to 8 digits for SG

Have to change regex already to maybe this d{8}?

can just initialise to private final Phone phone. Same for all classes. Just import the package at the start can alr

Same problem no need full package name

editedPerson change to editedFriend

Should change person to activity

Same

Wishlist

activities

Ya but i dont know what badges we should add, there's quite a few different one

Ok added

Changed

Okay changed!

This test will fail so i changed it back.

Ok resolved. Thanks!

Maybe change the sections (deliverables, contacts, meetings) to their respective epics, following our meeting docs? E.g. this line of code can be "[EPIC] As a Product Manager, I can track my product’s development so that I can work better towards production deadlines. (Deliverables)"

Noticed you left out type of Product Managers in here and elsewhere (e.g. "Product Manager who needs to be aware of changes in description of deliverables"). I believe we can leave these out for now. Just a note.

For consistency, can we change to just "mark deliverables as completed"? (I'll update the changes on the docs)

Can we change to small caps 'm' for 'Minimise'?

In here and elsewhere, should we follow the user stories as closely as the gdoc since it's more or less cross-checked among ourselves? i.e. for this line of code, can it be 'contact the right person if needed'? Alternatively, 'locating the right person easily' would work, since this user story is about editing the POC.

Should we take out "(completed deliverables / total deliverables)" since it may lead to confusion? It may be misunderstood as completed deliverables or total deliverables instead of over. Alternatively to explicitly state "completed deliverables out of total deliverables".

For consistency, can we have "keep them up-to-date" for the 'so that' portion?

Can we have back the 'remember and retrieve important information' under 'so that' portion?

Can we capitalise the 'i' in 'keep any data that i want'?

Can we have plural 'contacts' instead of 'contact' under 'so that' portion?

Did you miss the epic "As an inexperienced or forgetful Product Manager, I can refer to a user guide as I’m using the app so that I am able to use it as intended"? The user stories in this epic are not reflected.

Here and elsewhere, noticed the numbering isn't correct, but I believe markdown solves this automatically. Have to check again? Just a note.

In here and elsewhere, for consistency, can we ensure all the lines end with periods (.)?

Can we ensure the use case IDs are in consecutive order, i.e. this should be UC11 and delete contact should be UC12?

Can we change to singular 'Deliverable'? (side note: we may need to redefine this keyword. Personally it sounds slightly vague though I know this definition was taken from online).

Should we leave this keyphrase out since I don't think we use it anywhere?

Can periods be included for all the epics too?

Have seen that the rest have periods now. How about this line too?

I believe you added the 'so that' portion wrongly here. It should be for the "view my contacts and their relevant details" user story under EPIC B. Could you help to change that?

Could we change this to "view a helpful popup"? Just a small grammar error.

I think your suggestion would work for now.

Can we fix the typo here? Maybe in the next big commit for UG. Just to note.

I made two fields for Deadline class. One a String, and another a LocalDateTime. We can use the latter for comparison, and leave the former as it is for string rendering (y)

Will create another PR for this commit.

No need to review/merge.

Checks failing due to Edit command not available yet.

Fixes #3 and #5

Change e to to another more descriptive name?

Similarly for this, change e to a more descriptive name.

change address to description

Should we remove all the tutorials?

change to package seedu.momentum.logic.commands;

Do we really need to store the entire project object in a timer?

Maybe we generate and use an unique project id?

calculate looks quite long, maybe you can abstract out some of the logic here?

I think the @param should end with a full stop.

The method is > 30 lines. I see you tried to reduce the length of the method with combinePredicates but I think you can abstract more of the logic.

Similarly, full stop for @param

Here also the full stop for @param

Maybe you can store "Kurz Elle Kunz" as a String variable since you used it more than once in this class?

Similarly for the starbucks elephant cats here

And friends owesMoney here

Similarly for "\\s+"

Similarly for "Alice Bob"

Store the array as a private static variable also?

Also for the "Alice Bob" in this class

Maybe change the getFormatted method in deadline to return no deadline set when there is no deadline instead so that a project does not have to know whether it has a deadline.

fixed

I think 8 spaces not 4. Changed to 8 spaces.

Hmm I don't think is matters

Name is not optional right?

maybe just include as coming soon

Fixed

Hmm not sure

Changed to edit

Yeahh we should

Yupp

Not sure, but if we add a bunch examples, then a table would be like 1 line | a lot of lines

Fixed

This is to show deletion of a deadline so the deadline "created" is an empty one.

It's just the work of reformat code to align with the next @param

Time is compulsory for LocalDateTime in java.time so unless a dummy time is added in, LocalDateTime cannot be used.

I think it would be cumbersome to try to differentiate between the dummy time and the actual time if any. I think this mainly has to do with the backend part.

Then since the time and date is separated, it would easier to parse them separately.

I think I will create a general date superclass then make datetime, date and time inherit from it.

After checking the hierarchy of java.time, I realised the only common class ancestor of Date, Time and DateTime is java.lang.Object but they do implement similar interfaces. So I decided to make Date, Time and DateTime to implement a common Instance interface instead of a common class. And also if Date and Time inherit from the same class, it would imply that Date and Time is comparable.

'''suggestion

Edits an existing tag in StonksBook to the specified tag name. All entries previously associated with this tag will be updated to associated with the updated tag.

'''

I think we can remove 'the' here

'''suggestion

All contacts that have been previously associated with this tag will be updated automatically to be associated with the updated tag.

'''

Should we mention friends here? I think it may be better if you just mention that it updates the name of the first tag (as per tag list)

Same comment for here (about whether to mention friends)

Maybe we should consider switching to an ordered list if we're going to make reference to other points

Think you're missing a Use case ends. here and in some other portions below as well

Can this be indented to be the same level as 2a? Likewise for the remaining use cases

'''suggestion

StonksBook adds the provided tag.

'''

'''suggestion

'''

I think here we're assuming that contacts are sorted before sales right? Should we add a comment stating this assumption?

Can we write this section into an if-else block with the above? Then we can also remove some duplicate code

Hmm, is associations a bit hard to understand here? Would it be better to be more verbose here and say something like Note that all items that have been previously associated with this tag will be updated automatically to be associated with the updated tag. as per the user guide?

P.S. May also need to update the user guide: it currently says contacts when it's supposed to be generic right?

Same feedback here as in DeleteCommand

Is this comment still relevant? If I'm not wrong it refers to the tags stored in the persons/sales right?

Javadocs here is inaccurate

Would it be better to initialise a new HashSet with the original tags, remove the target, then add the editedTag in order to prevent arrowhead-style code? Same can be done for the removeContactTag

This should be equivalent tag instead of person right?

Should be tags here instead of persons

Is this javadoc accurate?

p.s. i think these 2 lines are common across the conditionals. maybe we can extract it out of the conditional? same for EditCommand

Thanks for pointing that out!

Oh, my bad. I didn't know the new line would affect the formatting. Thought it was just code styling. Will remove it!

@Asthenosphere ps can help resolve the merge conflicts?

LGTM

Thanks!

Think we're all done with this. Nice work all!

I think you should keep "Developer".

I checked again, and I think you missed out the find sales history feature under sales tracking.

I agree.

I think that this should be specified as a sub feature under list, so as to distinguish between all ingredients and a single ingredient.

Perhaps this archive employees contact details section should be grouped with the other employee contact details features?

I think this page will be continually updated, so we should include what we have done to the project so far - which would be the documentation section.

For the ingredient levels, it would be good and clearer to state the unit of measurement. You could add this to the description or edit the examples to show this is the case.

We are missing Feature 4.3 Find the sales history data based on date documentation.

Remove the bullet points in this section to standardise the description field for the features. Instead of starting with a noun, start with a verb (once again to standardise across the UG). Eg. Set the level ...

Since it's just one argument, requireNonNull(targetIndex) will do.

I think we should keep both. They will both be Phone objects, but with different prefixes.

Perhaps you can use requireAllNonNull(...) instead.

Should this be using .equals method instead to do the check?

You are missing empty lines in between the methods.

IngredientList should not be under AddressBook, as AddressBook should only be in charge of contact details.

Mismatched javadoc?

As mentioned earlier, functions and features relating to ingredients tracking should be abstracted out to its own classes (instead of being in AddressBook).

I assume this has yet to be implemented?

Incorrect test case? Should these be all throwing assertion errors instead? They should not be called as it is a default model stub that have all of the methods failing.

Should target be non null too?

Requires change to fit ArchiveList

LGTM

Could you change this instance of 'AddressBook' to 'Athena'?

Change this instance of 'AddressBook' to 'Athena' as well

Adds an event instead of a event

Consider adding Javadocs to this method and the newly implemented methods below

What exactly is this change?

Please commit only relevant changes to the branch in the future. This is probably more relevant when adding the editEvent command

Add class-level javadocs for this class

Update this javadocs for the context of AddEventCommand

Use the // some comment for this instead of /* some comment */ as that is relevant only to the method/attribute right below it which in this case is PREFIX_DESCRIPTION

Please include class level Javadocs for this

Consider placing this into the constructor?

Update javadocs to specify calendar

Update Javadocs to specify it is about events not persons

Add class level javadocs to tell us about the class

Add class-level javadocs to tell us about the class

What is the difference between these two methods? Should the setEvent method check 'equality' between two events and replace the one in the list with the one outside?

I believe this replies to the wrong part of the review? Please use this only to reply to the specific comment above

Consider adding this method and the test methods below to a separate class called AddEventCommandTest.java to separate tests for Bangyi's AddCommand and tests for your AddEventCommand.

Consider adding these messages as constants instead of magic literals

Issue has been fixed! Thanks for the notification. I ticked the 'Require status checks to pass before merging' option without realizing that I had to specify which CIs I wanted passed before being able to merge. This has been addressed and our CI is now passing.

Is the sort method supposed to sort the current shown list? Or is it supposed to sort the full list of contacts before showing it? Because the former would be much easier to implement and I believe is what we intended to do originally? There is the model.getFilteredPersonList() method which gives the last shown list.

Also, I agree with the Hendey's comment about the sortPerson method. I do not think that all the model classes need to have such a method.

I think for this, it could just be "User enters the required details" since implementation details are not needed for use cases

Same as the above. Instead of this, perhaps you could try "User requests to cancel the command"?

I think for this, it could just be "User enters the required details" since implementation/UI details are not needed for use cases.

Same as above. Perhaps you could try "User requests to cancel the command" instead?

I believe bullet points are required for indentation here as well.

Perhaps a new indentation would be good here?

is there a typo here?

Might be better to mention that this command deletes everything before and up to the given date. It may be misunderstood to just delete visits on that date itself.

What might this suppress warning be for?

There shouldn't be a space after the method name. Also, since this method deletes multiple visits, maybe calling it deleteVisits would be more suitable.

This functionality could perhaps be moved into the for loop above? Then there would not be a need for an additional ArrayList to keep track of the visits to be deleted.

argMultimap.getPreamble().isEmpty() should be !argMultimap.getPreamble().isEmpty(), otherwise the code fails.

Only this part causes errors; the other comments are queries/small formatting errors.

The JavaDocs for this Predicate could be clearer as this Predicate is not as straightforward as the others.

Might be better to name the conditions separately with informative names as this line of code can get confusing.

Now that there are three parameters for the constructors, perhaps it could be better if each parameter is tested for NullPointerException in a separate method.

Typo here. Shouldn't be nullLocation but instead the parameter that is null in this method.

Shouldn't date be required to be non null as well?

Could this be moved to the ModelPredicate class as the predicates are being stored in that class at the moment?

Is there a typo in the method name?

Yes, these methods should already be functional barring unforeseen bugs.

The method was added for the following reasons:

The Person and Visit classes both have these two methods in Model - getFiltered[x]List() and updateFiltered[x]List() where x is either of those classes. As the InfoHandler class requires the getFilteredLocationList(), I felt it was apt that updateFilteredLocationList() was added as well.
Also, the addLocation method in the ModelManager requires the updateFilteredLocationList() as well.

As for the test above, the function is required to be overwritten for the ModelStub to be created.

Issues

Format

When creating an issue, try to specify the part of the project the issue is meant to be working on by using square brackets in the title. If the issue does not specify anything, it is assumed to be implementation-based, i.e., related to modifying AB3/VirusTracker code.

Example: [DG] Add UC01 and UC02 indicates that this issue works on adding user cases 01 and 02 to the developer guide.

Labeling

Mandatory label:

priority.* Indicates the priority level of the task. Has three levels, low, medium and high.

Apart from the mandatory priority label, you may include other labels to make it easier to search and locate issues. Feel free to add any labels, but please ensure no similar label already exists.

Pull Requests

General note: PRs can only be merged only when two or more other people have reviewed and approved of the merge.

Format for PR

When creating the PR, please state which issues the PR closes on the last line of the description.

Commit message: 'keyword #xx, keyword #yy, keyword #zz' for multiple issues

'keyword' refers to any one of the following three words, 'close', 'fix', 'resolve'.
- Using these keywords will cause the issue to be automatically closed once the PR is merged successfully.
- _Note: Other tenses for the above words could be used as well, such as 'closes' or 'closed', but let us standardise using the
  
  imperative form of the words above._
'#xx' references the issue number, where xx is the number. For example, xx = 10 will reference issue number 10.

Example: xx = 30

'''

...PR description here

Close #xx

'''

The above will close issue 30 upon the successful merging of the PR.

The actual form of #xx is not used in this comment as it would reference actual issues. Please refer to the PRs page for more examples.

The user cases have been added, and this issue will be closed

This user story has been added, and this issue will be closed.

Location classes have been completed, statistics classes will be moved to v1.2.

A new LocationHandler class should be created to be in charge of handling the statistics.

I've updated the title accordingly to avoid confusion

Rather than LocationHandler, we can instead create an InfoHandler class which additionally handles people and visits

Issue can be closed, InfoHandler class has been implemented with relevant methods.

Should this be Hall-y instead of AddressBook? Also, should it be hall admin (according to the use cases) instead of hall leader? Similarly for the rest of the terms below.

Should it be 'enters' instead?

Hmm seems like there was an issue with Git. Could you keep the original line under 'Saving the Data'?

Also, could you update the command summary at the bottom of the UG as well?

Should it be just 'in Hall-y', instead of 'in the Hall-y'?

Should it be resident instead of person?

Should this be a resident instead of a person?

Nice! @schoolex can refer to this for #37 when implementing the CLISyntax.

Should this be a series of instructions instead of a tip?

i.e. something like the edit command in the UG:

GENDER: must be either M or F
ROOM_NUMBER: must be <block cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf">_<room Number cwf="C:\repos\nus-cs2103\dashboards-base\contents\cs2103\tp-comments-panels.mbdf">

Should there be a space after the commas?

Should it be 'listing all residents' instead?

A very minor nit, but is there an additional linespace between the previous method and this method?

Should GenderType type be a private variable as stated by the coding standards?

Minor nit again 😟 . Is there an additional linespace between this if block and the previous one?

Should the getEditPersonDescriptorDetails() in the file be updated also?

Should this be GENDER_DESC_BOB instead?

Should the "M" be GENDER_DESC_BOB instead?

Would you consider adding a similar test for invalid gender as well?

Would you consider updating the MESSAGE_USAGE in this class as well?

Should the constructor of this class be updated as well?

Minor typo here in the comments.

Hmm the old link generates a badge though. Using /actions/ doesn't generate a badge, but a link instead...

Ohh I see. Sorry I misunderstood that.

Okay! I updated it. Currently it's just dummy data. We should probably have a source of truth somewhere in the code to validate if the block numbers and room numbers are all valid (I'm thinking somewhere in preferences.json might be good).

Closing this, since it is part of the tutorial. Will not be merging unless we decide to drop address.

Closing this, since it is part of the tutorial. Will not be merging unless we decide to add Remark.

Need to verify if the tP tracker will still pick this up if the PR is closed. Do reopen if the tP tracker requires the PR to be open for grading.

Closing since AB3 already implements this feature. Do make sure if that the feature works with the updates to #10.

Remove from milestone v1.2. Will consider adding this into milestone v1.3.

Oops I missed this out: could you also update the command summary section for the edit command too?

PR closed as I messed up Git. 😢

Closing this, since it is part of the tutorial. Will not be merging unless we decide to add Remark.

Not sure why the badge doesn't load when it's first loaded, but after refreshing, it loads? 😕

Closing. Has been completed.

Closing for now since the tP tracker says there are no references left.

DG might still have some (since the explanation of the code still needs to use AddressBook*.java), but that's fine.

"Question " but no biggie

"QUESTION " instead of "Name"

medmoriser**

medmoriser*

Medmoriser* methods

medmoriser*

medmoriser

medmoriserStorage*

medmoriser*

medmoriser list? question set list?

different question *

questions*

invalid question*

different question

wrong place

I think this should be Warehouse's remark 😃

Since we are including this constraint, should we include this inside our user guide so that users will be aware of this constraint when adding remarks to a warehouse?

Should this be Remark instead of Address? Noticed this issue in a few places below (email instead of remark, etc) as well for Javadocs of various methods. Do take a look at it 😃

Do you think replaceWarehouseList would be clearer here since the above method to change one warehouse differ only by a single letter? 😃

Should this be removed instead? 🤔

Minor issue, missing '.' for isSameWarehouse

Should we include warehouse here although for this iteration we are not intending to include tagging for warehouses products?

With the if condition statement removed here, will there be an issue where the for loop only runs one time since the return statement will force the function to stop executing?

Same issue here, as pointed out above.

Is the extra supplierList here as a placeholder for warehouse? I think the statusbarPlaceholder should be removed as well 😃

Just adding a note here to remember to change supplier to warehouse when updating tests cases later on.

Just to check, did you perhaps miss out the conflict here?

Same for these conflicted import statements as well 😃

Just wish to clarify, is the method empty because this method is allowed to be called but there is no specific function this method can do?

Alright, noted

Will it be clearer if the statement is Creates an Addcommand to add the {@code Product} to the specified {@code Supplier}

Maybe it will be clearer if you clarify that the exception is thrown due to index specified to be larger than list of supplier/warehouse since it is not so clear reading from these lines here 😃

Accepted, will remove @return and change "Creates" in first line of JavaDocs to "Returns"

Thanks for pointing that out, will push another commit to fix this!

Noted, thanks for pointing that out! Will be pushing another commit with the updates.

Thanks for pointing this out, missed out on the javadocs comment for class!

Noted, will updated in next commit as well.

Thanks for the suggestion, let me try to use that instead of arrays.aslist(String[]{})

Accepted suggestions above, will do the appropriate changes

Addressbook can be changed to CLI-nic

'serves'

name changed

Seems like this can be final also.

Perhaps the updatedQuantity should be renamed to productQuantity, because it seems like this should be a general constructor that future functions other than updateQuantityForWarehouse can use.

Since it is mentioned in your javadoc that this is meant specifically for suppliers, perhaps u can follow the naming convention that u did for toStringWarehouse. That being said, toStringForWarehouse may be better.

Perhaps this can be renamed to ALLOWED_ARGUMENTS or ALLOWED_ARGUMENT_LIST

I think there should be a \n here

same as above

perhaps its better code quality to explicitly specify the else with a separate if statement.

To do javadocs

comment should be updated as well

i think the naming for this is a bit confusing, at least at first glance, perhaps can change everything to all

I think you can just use list.of() instead of arrays.aslist(String[]{})

ab used to stand for addressbook, now it has no meaning so i think it should be changed

I think you might have forgotton to remove this.

Yes, It is the case. the latter is the stricter check.

Yup, its just a dummy function for the stub

LGTM

I don't think this line need to add UC2?

Might be better to change it to:

McGymmy shows a list of food in the database.

List of Food that user has added works too

LGTM

Just curious, what does this affect for the main page?

It kinda looks like a link to the repo like github.com/se-edu/addressbook-level3 and github.com/AY2021S1-CS2103T-W17-3/tp

Yes need the space after the ##

Might want to change this to a constant?

Like VALID_NAME_AMY, similar to the one u did in edit command test

Ookie in that case LTGM

I think we should use this.class.getName() instead and put it in the Macronutrient class and use it for each of the food items?

Might be able to clear up quite a few of the other assertions and methods mentioned below in Macronutrient

https://www.tutorialspoint.com/java/lang/class_getname.htm

Might not need this if we can get the nutrient type using the method mentioned in Carbohydrates.

This link might need to be changed in the future

Might be better to remove for now?

Might be better to put it into a method rather than just declaring

Can use @Before to initialise the fridge to reduce the complexity of layering the different test cases on top of one another (This might help if we decide to change/add more test cases in the future)

@Before

public void beforeEachTestMethod() {

    System.out.println("Invoked before each test method");

}

Might wanna remove this whitespace for consistency

I think there is alr a similar method in the current version

I think the standard for 2103 uses 8 spaces instead iirc. Same for Food.toString method

I think the test convention for other test cases are like getCalories_isCorrect or smth like that x.x

^^ Same comment as Joey

Might be better to abstract this.protein to getProtein(), same for carbs and fats

Might be able to use CommandTestUtil here to reduce code duplication.

Can compare the tags after the CommandTestUtil.assertCommandSuccess

What do you mean by this?

Ookie Thanks

Tags and Addr not implemented as they will be removed by the end of the iteration

I think the link for UserGuide should be this one? (https://ay2021s1-cs2103t-f13-4.github.io/tp/UserGuide.html)

The current one brings me to https://github.com/AY2021S1-CS2103T-F13-4/tp/blob/docs/UserGuide.html#quick-start instead.

Similar to UserGuide, I think the DG link is this one? (https://ay2021s1-cs2103t-f13-4.github.io/tp/DeveloperGuide.html)

Sorry, I think this 'food' is not changed yet.

Should we change address book to food inventory here also? Same for line 158, 162, 166.

Very minor and we are going to delete address anyway, but do you think this should be address instead of simplykitchen?

Should the "Persons" here be "Food Items" instead?

Should "persons" here be "food items" instead?

Very minor, but should we change "addressBook" parameter to "foodInventory" instead?

I think the "bar" in statusbarPlaceHolder should be "Bar" instead? Since it was changed to "Bar" in MainWindow.java.

Very minor, but should this be expectedFoodInventory instead? Line 112 and 117 as well?

Very minor, but should this be Food Inventory instead?

Should SimplyKitchen Inventory be Food Inventory instead? Line 137 as well.

Extremely minor, but should "an" be "a"? Also should "persons" be "foods" instead?

Very minor, but do you think we should switch back to Label for consistency?

Very minor, but should the comment for invalidity be different?

I feel that since priority is no longer optional, should we set the priority directly without the Optional here?

Should we have a priority missing test here as well?

Okay changed

Changed all comments with name to description.

I agree, changed them. Thanks!

Yeap I agree. Changed them.

Might want to take in the user input and array of prefix, and store it as a instance variable.

No parameters needed, use the instance variable values, SLAP.

change to private

change to prefix

change to private, and change the name of the method, very similar to the one below

change to private, change the name.

Pull it out as a separate class.

SLAP, Abstract out the new ContactList creation.

Wrong object used, should be ResultCommand

Why need isSameContact method and the overwritten equals method?

Is it cos in the event of 2 differnet person with the exact same name?

Should it be final? Might be hard to keep updating right?

Will it work? Since this.contacts is final.

Same thing, contacts is final, im not sure if you can just set it.

Follow SLAP, bring out the remove function call.

Might want to pull this out into a new class for common usage. As I foresee we will be using this for some of the other classes as well.

I will fix in the latest update!

Looks good to merge, just file renaming.

Should we keep this?

maybe, coming in mid-v1.2? 😄

Don't remove this part. It is used by Jekyll

Perhaps, if you add this then we will have double title on the page (?)

Is this more of a "List a Client" use case?

'''suggestion

FitEgo adds the Client

'''

I'm not too sure if there is a timestamp in the status bar 😅 , but it is also in the AB3 DG ...

'''suggestion

  Expected: First contact is added to the list. Details of the added contact shown in the status message. Timestamp in the status bar is updated.

'''

'''suggestion

FitEgo displays the client's whose name matches the keyword or text.

'''

I see similar issues in other place as well

Perhaps its better to move this to ViewClientCommand::execute?

as to maintain separation between Ui and Logic components?

I agree with Dhafin.

i think in a javascript world, it is called a callback.

Perhaps hasCallback and getCallback is a better name?

Sounds good!

[open for discussion] Is it better to do the new way or the previous way?

This change is related to changes in SessionBuilder::withInterval

cc @maguireong @dhafinrazaq

Sorting session by its natural order is from the oldest (?)

Iirc, we are not using idCounter anymore for sessions

Perhaps its better to just delete this and add a reminder in the issue, I think

hmm, what happens if we remove !important here? 🤔

Oh okay, i think I'll change it to only allow for the case where

cannot start or end with -
allow multiple - usecases

Oh right, I'll make it private

I think it is better to keep the structure of AB3 UG for future reference

donee

Proposed changes: show the number of clients inside the UI

TODO:

Repurpose the tag to history of injury / allergies
Change test data to medical names

Done for v1.1. Closing..

@dhafinrazaq @maguireong can help to look at it?

@tanweijie123 will this change be compatible with the change you proposed for #68 ?

Btw, perhaps it is better to test for checkstyle locally before pushing?

To run checkstyle and test cases, run ./gradlew check from Intellij terminal

To edit your last commit (so as to clutter the commit history), do git commit --amend --no-edit (also in terminal)

Yes, I propose this implementation:

sdel INDEX : will delete only if there is no schedule tied to that session
sdel INDEX f/ true: will force delete all schedule tied to that session and the session itself.

Implementation:

Will look for all schedules tied to the corresponding session before deleting the session

If i read it correctly, changes to existing classes is to enable comparing sorting clients (compared by name) and sessions (compared by interval start time) right?

Have we checked with Prof Damith on using controlsfx as external libraries?

should this be mid-v1.2?

okay agree

I got the same warning and also mentioned here: https://github.com/nus-cs2103-AY2021S1/forum/issues/232

(it's an error log output). So this should be fine

Should the parameter name be client instead of Client?

Should the parameter name be clients?

should Clients be clients?

ohh, if the checkstyle version/file in both GitHub and IntelliJ are already configured properly but the outputs are different, I'm not sure which one to follow. Perhaps @benclmnt have a suggestion regarding this code quality?

I think this is what wei jie means. Perhaps you can change these jsons from "Clients" to "clients"

Or should we ignore the GitHub's checkstyle warning for now and merge, and fix it along the way?

Should this be FitEgo?

Should this be Clients or clients? (for consistency)

should the name be something like "hasFunctionToRun"?

Should this be non-public?

should this be session builder? noticed the same cases in this file as well

should the variable name index be changed? since there are 2 "index"

just curious, any reason why this takes in LocalDateTime instead of String (since the other with__ takes in string)?

in terms of consistency with the other with__ methods, should we pass the String? What do you all think?

Is it better if I change it into:

Use case: UC03 - Edit a Client

MSS:

    1. User requests a list of Clients using `clist` (UC02) or `cfind` (UC04) 

    ..the rest

done

should the find feature be implemented later? Also, the current AB3 only supports search by name

Is this like adding a new command? If yes, should this be for v1.2 or mid-v1.2?

The division of 3 sections is in the other PR. Should we bold the nextSession line?

sorry just read this. both bolded/not bolded looks good for now

Some things to take note:

1. Command has been changed from "[c/CLIENT] [s/SESSION]" to "[INDEX] [s/SESSION]" which makes more sense.

2. 1 issue which is that when the input session is higher than the total number of session, there's an exception on the terminal but nothing is being printed on FitEgo. The issue is with RescheduleCommand only having the [INDEX] which takes care of the problem when [INDEX] > total schedules, but in Parser, I'm not able to retrieve the filteredSessionList as it requires Model to retrieve it, so FitEgo can't detect the issue when input session is higher than total sessions.

3. Testing for this Reschedule is not fully up yet because there isn't a ScheduleCommandTestUtil class, not sure if this is required because now it's a bit tricky in a sense that Schedule contains Client and Session while RescheduleDescriptor contains clientIndex and sessionIndex of Index class. If RescheduleDescriptor were to contain Client and Session, it should be easier but in RescheduleCommandParser, similar to issue 2 above, I'm not able to retrieve the filteredSessionList as it requires Model to retrieve it.

try to detect the issue in command.execute() instead of parser?

A good feature to have. will the implementation be like inside DeleteSessionCommand, iterate through the model's ScheduleList?

looks good

Will removing the "find Grab" example make the table look neater?

Are we missing the details for adding an expenditure?

Perhaps "Temasek Hall" is too specific to NUS? Users who are not from NUS may not understand what it means. "Daily Expenses" might be a better example. I noticed the same issue in several other places.

Will it be more concise to use "budget" instead of "budget book"? I noticed the same issue in several other places.

Will it be better if "must be a positive integer" is bolded to draw users' attention to this restriction?

Same issue as above regarding the bolding of "must be a positive integer or double with a maximum of 2 decimal places". There seems to be two spaces between "of" and "2".

Same issue as above.

Perhaps it might be more cohesive to capitalise "school fees"? I noticed the same issue in several other places.

Yup, let's use "budget" instead of "budget book" as discussed. Good catch 👍

There is a missing space between the and LogicManager.java.

Perhaps DevOps should be separated with a white space to follow the title?

Perhaps it is more appropriate if the method "isValidBudgetName" is changed to "isValid"?

Perhaps it is more appropriate if the argument "budgetName" is changed to "name"?

Should this be ExpenditureCard instead?

Perhaps it will be more OOP if there is an additional private method to get the the budget of a certain index.

Same issue here.

If you want, you can re-upload your photo so that it scales correctly. @sogggy

The examples for adding an expenditure is not in the correct format.

Did you see TCQian.md?

May need to align capitalization?

Would it be better to put INDEX1 and INDEX2 as code (i.e. in ``)

May quote INDEX as code

May quote 'list' as code?

deadline number may be changed? e.g. deadline datetime

Similar as above

This may need to be changed?

I can see that all your code works. But would it be easier if we use java LocalDate and LocalDateTime to deal with it? And it may be easier for us in the future work if we want to filter projects by deadline.

All the examples may need to updated according to the validation regex.

This file may need to be changed (for deadline regex)

I think we'd better keep the format aligned, say use DEADLINE_DESC_AMY as the name. And we can change AMY to project names together for future refactoring work.

Same as the above comments about naming

Same as above comments about naming.

This is an email address rather than a repo address right?

Slightly confused why the need to update the prevous code base person model. Is it failing checkstyle?

Change to Tests to follow the rest of the jdoc comments.

Could add constructor tests eg: Referencing from person/AddressTest, as constructor has requireAllNonNull similar to person.Address

This could be applied to a lot of previously seen files.

Something like this is possible:

'''suggestion

@Test

public void constructor_null_throwsNullPointerException() {

    assertThrows(NullPointerException.class, () -> new Location(null));

}



@Test

public void constructor_invalidAddress_throwsIllegalArgumentException() {

    String invalidLocation = "";

    assertThrows(IllegalArgumentException.class, () -> new Location(invalidLocation));

}

/**

'''

Add null location test, referencing: UniquePersonListTest.contains_nullPerson_throwsNullPointerException()

Similar as first comment about Tests that -issue-

Same as abv

Slight complaint but could add line comments on what exact part of equality is being tested in each assert as it was done in some other class equality testcases

On further review, this addition would be a big increase in requirements for testcasing and is not suitable for a small modification to a completed pull request.

We should open a new issue about this on refactoring and improving testcase coverage on Constructors and Null Checks.

Perhaps some jdocs here would be good to ensure readability of code

Jdocs

Maybe should have a MESSAGE_USAGE string like if someone does sth like

listi jkdnkjvsdsn

or something ( unsure if will return error here)

Constructs ...

May not be in line with good OOP practices. Consider alternative representation or if it is not the proper namespace usage, amend

Bit more spacing would be good to have and perhaps line commets

Same as abv

be careful what you wish for 😃

adding these lines back cause checkstyle to fail somehow?

Will not touch. should be handled by #20

Inventory is meant to refer to entire state of the inventoryinator. will add term to glossary

Oops :p lmao

Not known how implementation will be done, hence maintaining similar format as prior implementation for now.

Oof

User Guide is done for now.

From commit 9b2ccce:

In process of Unittesting, dificulty was encountered from utilising

testing utilities of original code base.

Specifically the assertCommandSuccess testutil method, relies on the .equals

of model manager, and model manager equality has null values initialised for AB3

and Inventoryinator versions, which if implemented to check, cause testcase

failures in stubs.

Recommended to perform refactor to code base and remove ab3 artifacts, for more effective

testing.

Will open new issue based on this finding.

Related to

#20

index.getOneBased() - 1 is just index.getZeroBased()?

what is this haha

toAdd is can get confusing here, consider another name (deletedRecipe/updatedRecipe/etc)?

Do we need PersonListCard.fxml?

What about recipes?

Might be good to add an example like you did with Item

your optimal size / an optimal size / the optimum size might be better

Should a TODO be placed here?

Might be good to have TODO here as well

my zheng in the filename is missing a g lmao

might want to update the png as well

same here

might want to update the commands

same here

Will there be an item named alice bob charlie?

And @seanjyjy I think you are missing this statement header.

InternHunter allows the management of three data types:

I don't think there should be a guarantee here that the system stops running since extension at 2a implies that use case ends when user decides to cancel the confirmation

I think ProfileItem here would suffice instead of the import

@seanjyjy @orzymandias @ZoroarkDarkrai @shawn-nyk

I think this is a good topic for discussion. Currently, AB3 only uses the weaker notion of equality, isSameItem for the edit command. The add command on the other hand checks based on the equals method. Do we want to change it such that it uses the same notion of equality as well?

I don't see the need for this class as it is the same as the Name class, we could just pass in the messageConstraints to signify a different error message if need to.

Perhaps an accidental double lines here?

Yes, we would need to pass in the messageConstraints variable to the parent Name class. Yes I think the code is good enough to merge!

I was thinking for descriptors - equivalent of the skills in internship should just be a non-empty string! Because I don't think there are any wrong formats that we should disallow them from inputting?

I think it would be better to put in C# instead of C sharp? Since its easier to put in. Not sure about the char limit though, since some companies may have very long names?

Perhaps we can include a check for when the user is already currently viewing this current item and display a MESSAGE_ALREADY_VIEWING message?

I think that it might be better if we initialize the variables in the if-else block, so that it is clearer which block will have what booleans. And also this will stick to the notion of defining the variables in the least possible scope!

I think the current code is better. Also, why is there checks for tabName in this command? Since all the other commands automatically switches to the correct tab, even if it is already on the right tab

Oh what I meant was initializing the value in the if-else block and not declaring it there. So an example of this is:

Oh okay thanks for the clarification!

Don't think so, since he did mention that this is just a guide and I remembered there was an example which he said would be ok?

The job list in the company has the name of the job, so I don't think this will be an issue.

Good catch, updated it already. Thanks

Yes, I did thought of this while I was writing the code for this Job class. Yes, I think that we may need need a Job object and it can be represented as a String instead. Will edit the code accordingly. Great catch on this!

Yes haha, I had a good laugh earlier when I represented wage as a Phone object. Reason I did so was to use AB3's current implementation of the Name object (which is basically a class with a string containing only AlphaNumeric characters) and Phone object (which is essentially a class with a string containing only Numeric characters). I think that we can rename it in the near future and make use of them in all our classes. What do you think?

Yup, I think its best if we could ensure that the "Job" and "Internship" names are not used loosely throughout the code base. I don't particularly like the idea of a nested class, since I do intend to use the enum outside of this Status class (for parsing and input). Also, would it better to rename the Status class as ApplicationStatus and the enum to be called Status instead?

Kind of, the date will be used on the main display as well as on the right pane:

@ZoroarkDarkrai @orzymandias @seanjyjy What is the team's opinion on this?

@seanjyjy @ZoroarkDarkrai @orzymandias Does anyone have any good suggestions? I'm leaning onto Status composing ApplicationStatus and Date atm.

Thanks for the input. I created a Status class to abstract out the application status and date details from the internship. But perhaps now that I think about it simply renaming the fields to InternshipStatus and StatusDate would do. Let me know if this arrangement is fine.

Yes, thanks for pointing out this error

Yup made the change

Yes, thanks for noticing.

Okay, updated for consistency

Yes, I agree that tabs actually make more sense in this context. Will update for all use cases

Right now we are only allowing one application for an internship. So perhaps your suggestion may be quite misleading too.

I have updated to:

The application (if any) made with this internship will also be deleted.

Hopefully its clearer this way!

Right now it is safe, since i think only @seanjyjy and I are using it. Thanks for the catch though, I will add an assertion there in case someone else happens to use it on a string with length <=1

Yes will update

Yes, I agree with @seanjyjy on this. Since this is something within the programmer's control.

deepDelete uses the isSameItem method defined in the abstract Item class, which compares 2 items based on the weaker notion of equality, which I defined it to be comparing only the internship item. Hence why I created the deepDelete method! I will change the method name to deleteSameItem, hopefully its clearer that way.

Edited the names of the classes and methods and javadocs for increased clarity, do help to check before merging!

Not sure what is the group's consensus on this, so I took the easier path first which is to switch tabs after successful command. Could it be because the command is executed unsuccessfully? Because this should be correct since I took this from @seanjyjy switch tab command.

I think we could just set the default value for isSwitchTab in the constructor to be true then?

LGTM

Yup I think that there isn't a need to concern about javadocs / comments as of now as they can be easily fixed at any point in time. @orzymandias I stopped editing the List portions as I noticed that there were too much code duplication with your pr. Do we plan to either extend an Item class or use generics for our list?

ecessary duplication for collections, generics is a good idea for collections but one issue is with throwing custom exceptions for different types of items within the generic list. Should we just have DuplicateItemException then something like throw new DuplicateItemException(T.getDuplicateExceptionMsg())?

I don't think this works because we are not able to access methods in java generics. I figured since we all are classifying our classes as Item, we can have an Item interface to contain all the methods similar methods like isSameItem and likewise do throw new DuplicateItemException(item.getItemName())?

Sure, I think this is a solid idea.

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I think it's okay just I find many hard coded strings can be converted to const. If urgent can merge now and fix later?

Is it not normal to use hard-coded strings for test cases? Could you point on where exactly is the issue? Thanks

I mean the toString, equality, hashcode tests. Could use VALID_WAGE, VALID_PERIOD, etc? If it's a better practice to use "Sunday", "React Native", etc. it's fine then.

Not too sure what's the correct which is the correct way, but I just thought that doing it this way by writing 2 strings that are different objects would be better since we will be comparing periods with different strings in the actual implementation too

Might need to change the URL for the badge!

Oops! I think the benefit for this is wrong!

Also, I was wondering if we should combine the benefits stated in potential users and use the combined version for this user story?

Apart from this, everything LGTM!

Just a minor issue but perhaps this @return should be removed as well!

Oops minor typo here!

I think this should be removed as well!

Perhaps it might be better to change this to a more warehouse-like-name to avoid any potential confusion! Wondering if we need to change the variable name as well?

Do we also need to change the variable name here as well?

Here too!

Oops traces of supplier here!

This part here as well!

This should be AddProductCommand!

I think this message could be clearer! Perhaps something along the lines of "Tag names should contain only 1 word"!

Perhaps this could be "Adds a product to a supplier in the CLI-nic app"!

Just wondering if this violates the law of Demeter?

i think this is ok but we should have a command to edit start and end date within travel plan directory also if not it might be a bit confusing

can change person to activity

rename p to activity?

person to activity

AddressBook change to ActivityList?

addressbook change to activity list?

address book change to friend list?

Returns true?

AddressBook change to WishList?

change person to activity

person change to activity, addressbook change to wishlist

addressbook change to wishlist

think you accidentally added 'a'?

Also added delete command to user guide. Not sure why it wasn't there? Maybe got removed accidentally.

Don't think need to compare caloriesPerRep, since it is not an identifier field

This property should be called value

getName().name sounds a bit unsemantic as the property name has the same name as class name (haha so many names)

This test could be removed, see previous review

Could rename to calories for clarity

What do u mean by hardcoded here?

should be setLogs?

need to fix the naming of the overloaded method below too

should be find w/o s

can split this into 2 tests

find_state_expected

Can split into multiple tests with format method_state_expected

Can split into multiple tests

with format

method_state_expected

Remember to change addressBookStorage to logBookStorage (same for the lines below)

Should update to use LogBook

Person -> Log

Should be logBook

LogBook for the return type

Closes #17

Not needed anymore

Should be added to Table of Contents as well if we're adding in a new command list

The table of contents is actually generated by GitHub with

'''

Table of Contents

'''

With this addition we will have two copies of the TOC on our documentation website.

While editing this, could you also change the spelling of "Travelling Businessman" in "Glossary" to have a capitalized "M"?

Minor point, but can this be changed to "E.g." instead of "eg" with the period.

'''suggestion

        + "client edit INDEX [n/NAME] [p/PHONE] [e/EMAIL] [a/ADDRESS] [c/COUNTRY] [tz/TIMEZONE]\n"

'''

Should this be checked with requireNonNull(predicate)?

String can be put into a constant for clarity

Similarly, is requireNonNull needed here?

This check seems to be carried out in ParserUtil already

'''suggestion

/** Client command in the form client view index, where index points to the index of the client at current location

of the rendered */

'''

Should fill in the javadoc here

'''suggestion

/** Client command in the form client view index, where index points to the index of the client at current location

'''

To be more inline with the coding standards

Could you change the class and file name to ClientViewCommandParser? I will be updating the ones for Find and Delete in a future PR

Missing javadoc for return here too

'''suggestion

/** Client command in the form client view index, where index points to the index of the client at current location

'''

Pls have the space there 😦

I'll update for delete but leave find as it is for now, I think someone else is working on find?

I think we can leave that to the refactoring since the hashmap currently uses String as the key

Initially it was because ClientUtil.java needed to build the command given the client's details, I changed the implementation of getClientDetails to explicitly use getCountryCode() so this function isn't currently in use now. I thought it would make sense to have countryCode since that is the value passed into the object like the other fields.

Fixed

ok, i've added it in.

actually, i just checked, it's supposed to be part of my other issue LOL

While updating this, could you add in numbering for the use cases as well? Then this statement can refer to other use cases as an example.

Does #121 branch continue from this PR? If it does this PR can be closed.

Resolved in #117

Resolved in #119 #131 #137

Perhaps "contacts" can be changed to "students"?

Perhaps "contact" can be changed to "student"?

Perhaps "contacts" can be changed to "students"?

Perhaps "person" can be changed to "student"? I noticed the same issue in several other places too.

Perhaps the email can end with "@u.nus.edu" instead?

Should this be "{@ MatriculationNumber}" instead of "Email"?

Perhaps the space between "Dangerous" and "Command" can be removed?

Should "studentn" be "student instead?

Should people be "student' instead?

Should the constant messages be renamed as well? I've noticed this issue in other places as well.

Should "AddressBook" be refactored to "StudentList"?

Should the parameter be change to studentList instead?

Should this be change to ATAS instead?

Should "AddressBookStorage" be change to "atasStorage" instead?

Good suggestion. But I felt that since the other attributes like showHelp and exit are action words, perhaps "switchTab" might be a better name to accurately represent what CommandResult does? Is there a reason why you think "tab" might be a better name?

Great suggestion! I will incorporate a getValue() method as u suggested above as well.

Yes, that is a good suggestion! I have incorporated into my new commit.

Right. Thanks for pointing it out!

Would you mind adding documentation to this? So that others will understand what this method is doing.

Do you think overriding the equals() is better?

Would it be better to change the method name to getTagName? This is for consistency purposes.

Could you change the documentation? This is so that others can understand what this class is about.

Should we start renaming the other classes soon?

Why is this deletable though? I thought this was the initial data to be used to populate a brand new instance of QuickCache

Should we should rename these methods as getQuestionAsList and getQuestionAsString? Might be less confusing in the future.

Is there a better way to represent this instead of question.size() == 1? Maybe abstract it into a separate method for this check?

I initially named it as content because I thought it would reduce confusion from having too many "choice" names.

But choice works fine too.

alright cools

okay great

alright!

good suggestion. will change it

fixed in the latest commit!

Let me update the UG for the find command first before merging the PR

UserGuide updated. Ready for review and merge

Updated my user guide in my commit 596b8cdc37112c815fbe794de967e5521dc4edad

I interpreted step 2 as confirmation message (success message); is this what you intended? To redirect to 2 if there is incorrect input?

(Because the google docs says resume at step 1)

Applies to all the other use cases too

Javadocs to be updated {@code Person}

Same for this, perhaps this is MESSAGE_DUPLICATE_GROUP?

Is this to check whether PREFIX_PATH is present?

Should this be removed?

Modify javadocs

Are you calling Serenity() ? (empty constructor)

Empty constructor

This feels very similar to equals()

This cannot be deleted (their requirements)

This too

Consider using student.containsStudent(toMakrAtt) instead of running studentsInfo.get(i) again

Consider using immutable design?

Since ParserUtil.java is still there, is it better to keep this test file instead of deleting it?

What's wrong with this test

Does the position of the unnamed parameter matter?

How does the program behave towards these commands:

edit -n JohnDoe 1

edit1 -n JohnDoe

Just to justify my curiosity, can a command have multiple unnamed parameters?

I think this should be AddCommand.COMMAND_WORD instead of "add", the same for other commands

Although these test classes will be changed in McGymmy, is it better to keep them just for this PR to make sure that the new parser is working properly with the old AddressBook test first?

Same comment as AddCommandTest.java

Maybe add some more tests for the old address book's logic, to make sure that it works exactly as expected as the old parser?

I think naming it toString is enough

I think naming it getCalories is enough

object equality is intended, as I was checking if fridge and expected fridge contains the same elements. I have overridden the equals method for fridge class

Ok I will change it that way

Oops, I forgot to implement it. I will implement it in the next commits

Ok I will change it

I think we should keep it that way to prevent potential typos. Instead of that, we can add a comment at the end of the line to explicitly state what Carbohydrate.class.getName() is

I was just changing the name of the class. I haven't changed any of its implementations.

Ok I will change it

Fixed

Actually, we cannot call this.class before the object is initiated. Therefore, this.class.getName() cannot be put in either Macronutrient's constructor or a static method.

I think one alternate option is that we can use this.class.getName() as a replacement for the macronutrientType attribute

Got it!

Fixed

LGTM.

The CI test says that you are missing the shadow plugins

I think your use case have the same id as sean's.

LGTM.

Should we use Index to select the book instead of the name of the book? Since the delete command already makes use of Index and using bookname is quite prone to typos

The user guide link is wrong, the correct link should be https://ay2021s1-cs2103t-f13-2.github.io/tp/UserGuide.html

Same for the dev guide as well, correct link should be https://ay2021s1-cs2103t-f13-2.github.io/tp/DeveloperGuide.html

I think you may have missed out on this part. The View command is meant to view detailed info regarding a single book.

I think this extension 2a isn't supposed to be here anymore, its included in UC05 already.

Just saw that the extension numbering here is incorrect, should be 2a, 2a1, 2b etc. Can you help change them too 🙏 thanks!

According to the user guide, the prefix should be "tp/" for total pages and "b/" for bookmark for consistency purposes right? Maybe we can discuss this further!

Thanks for doing the renaming! There's some parts which haven't been renamed such as the Javadoc here, comments in the equals() test as well as here: https://github.com/AY2021S1-CS2103T-F13-2/tp/blob/c568a3c5177c68a33c3e6279923fa8f3c0356df0/src/test/java/seedu/bookmark/logic/commands/AddCommandTest.java#L153

Would be great if you could change those too, thanks!

Could you edit the javadoc and comments as well please, thanks!

I think this line can be removed since the tests have been updated.

Could you update the tests names as well

I think there's a typo here, should be viewFirstCommand and viewSecondCommand right?

Perhaps you could include a super simple javadoc for this class?

LGTM

@mgiang2015 you uploaded your photo in the wrong folder, it should be uploaded in docs/images. Please open another PR to close this issue! 🙏

LGTM!

LGTM

Closed by #63

Reopening as @mgiang2015 's part on user stories for the View command is not done yet

Closed by PR #88

If I'm not wrong the textbook didn't have these white spaces.

Same problem as above.

Can delete white space, seems unnecessary.

Delete this line, I think its from a merge conflict.

Delete this single white space.

Good that you have a separate class for Description, instead of a String.

Good that you have a Predicate class

Maybe, abstract away details, for better readability.

Can you just combine the two methods, seems like not much of a difference.

Abstract away to reduce complicated expressions

Do you want to have this as a separate constructor instead. So you would have Room(int roomNumber, Patient patient) and Room(int roomNumber, Patient patient, TaskList taskList).

Good for changing the toString method accordingly.

Good for changing the equals method.

Okay I will change accordingly!

Hi, yes that's why I thought a PriorityQueue would lead to an easier implementation as we can determine its priority in the Room class and we don't have to individually compare details. If you really want an arraylist I can change it though.

It is to add the number of rooms into the hotel

It is to check for the empty room with the least ID number. So the first room, is the least number that is not occupied.

Or if it is occupied then every room is occupied.

I thought of that, but why would anyone want to input a negative number?

Yes, it is for future person assigned to this task to complete it.

but it passed checkstyle

I just followed the name of files given above.

Okay

I dont get your problem here.

It wont because 1 and 3 are stored in the hard disk

I think you are confused because they are methods in different classes, just like the original code.

yeap, im not responsible for tasks and adding person

okay sure ill add it

I thought AddRoom would be better suited for adding person to room.

Same reason stated above.

I'll change it if you want.

I removed that field.

okay

Can you clarify?

Actually I no longer have this.

I see a lot of you have a lot of questions on PQ. I just thought that the comparison would be simpler as the priority is maintained by the PQ, and all we have to do is to poll and we dont have to compare a lot of variables so fewer bugs.

Yes

Okay, will add!

Okay

I need it for the Json.

Okay will do!

Okay

That was wat I was wondering too, but didn't want to delete before discussing.

Okay!

Okay I will change it.

Okay.

Okay

Okay will add, thanks for pointing out.

Okay

Okay, thanks for noticing.

Please don't merge even if there are 2 approvals. Thank you!

Perhaps "DOB must not be a date in the future" will be better and more succinct?

Perhaps it is better to name it VALID_DATE_OF_BIRTH_AMY and VALID_DATE_OF_BIRTH_BOB to keep it consistent with the current style, where the full class name is used instead of abbreviations?

Perhaps it is better to name it DATE_OF_BIRTH_DESC_AMY and DATE_OF_BIRTH_DESC_BOB?

Maybe it is better to use the full class name for clarity?

A good case of invalid check since '/' is quite a common way of inputting dates.

Might have added 12 spaces instead of 8 by mistake?

Maybe you can re-arrange the order of the fields to make it more consistent with the rest of the code

Fixed

Perhaps it is better to have the first sentence of a javadoc on a single line.

Perhaps it is better to have these fields as private and provide getters for encapsulation purposes.

Perhaps it is better to define these variables as named constants (private static final) at the class level.

ok we can kiv

I have tried appending the commend to the end of the line, but the line will exceed 120 chars. Hence, I moved it to the top.

Thank you for pointing that out. I have added it into the check.

Completed fields necessary for v1.1

this should be email address

should the repository link in line 11 be changed as well?

might be more intuitive if the variable name is the same as the task name?

this function works in Tr4cker too. maybe we should keep the corresponding version of this line?

this might be an extra line

might want to surround the "prepare for tp tasks" with quotation marks

may want to keep the t in task as lowercase

should this keep the private access, and methods that use this call fromZeroBased(int) or fromOneBased(int) ?

would it be better to wrap the date time inside deadline with another class. so that we can just .dateTime instead of calling toString()

could name it MESSAGE_FUTURE_CONSTRAINS to be clearer

could these 2 if blocks be abstracted out? perhaps a method to check if the dayOfDeadline is contained inside the hashmap

if ur checking separately for TODAY, perhaps TODAY does not need to be inside the hashmap?

should be named getTr4cker_success() or something that implies test

Merged #19

Merged #12

Would it be better if there was no need to generate default serial number here? As I feel it is an extra step before we get the actual serial number for the stock. I may have an idea of how to do it, and can help to work on it!

Perhaps could remove this line as it seems it is not relevant!

Perhaps it is not needed to show the path to the serial number sets book?

This line is pretty long. Perhaps could break it down to improve code readability?

Perhaps could break down into more readable statements. Seen a few times!

As mentioned, perhaps it is possible to remove this method!

Perhaps could make a new exception class for the duplicate source?

Perhaps "stock" should be "serial number set"?

Should "inputted" be "input" instead?

Should this line follow the format of the others such as what is shown in "status message"?

Should we standardise past tense / present tense? ("Update" vs "Added")

Based on our last meeting, I think prefix for quantity to increment/decrement should be changed to "iq/"?

I agree!

Thanks! Have done so in newest commit.

To check with prof regarding the changes to be made to monitor code coverage

Minor issue, remember to capitalize Supplier and line 276 and 280 as well.

Same issue, minor typo for Supplier

Capitalize also

Similar to comments left on UniqueWarehouseList, should this be removed instead?

Similar to comments left on UniqueWarehouseList, might want to change to replaceSupplier

Minor issue, capitalise Suppliers

Capitalise Suppliers here

Would this mean that 0 is acceptable? Or is isNonZeroUnsignedInteger meant to be a stricter check?

Oh I see that it is being used for index and quantity separately in another file.

Alright got it!

Message should be "Deleted Warehouse: %1$s" instead

Hmmm... Okay I'm including these line breaks,

Thanks for pointing it out, I do agree with you and have specified in the next commit!

Approve for testing purpose to see the badge shows CI fail

Test successful, icon changed to failing.

@zhengweii Revert back to pass CI